Re: [PATCH block/for-linus] writeback: fix a subtle race condition in I_DIRTY clearing

[Jens Axboe]

On 2014-11-04 10:34, Tejun Heo wrote:
> On Fri, Oct 24, 2014 at 03:38:21PM -0400, Tejun Heo wrote:
> > After invoking ->dirty_inode(), __mark_inode_dirty() does smp_mb() and
> > tests inode->i_state locklessly to see whether it already has all the
> > necessary I_DIRTY bits set.  The comment above the barrier doesn't
> > contain any useful information - memory barriers can't ensure "changes
> > are seen by all cpus" by itself.
> >
> > And it sure enough was broken.  Please consider the following
> > scenario.
> >
> >   CPU 0					CPU 1
> >   -------------------------------------------------------------------------------
> >
> > 					enters __writeback_single_inode()
> > 					grabs inode->i_lock
> > 					tests PAGECACHE_TAG_DIRTY which is clear
> >   enters __set_page_dirty()
> >   grabs mapping->tree_lock
> >   sets PAGECACHE_TAG_DIRTY
> >   releases mapping->tree_lock
> >   leaves __set_page_dirty()
> >
> >   enters __mark_inode_dirty()
> >   smp_mb()
> >   sees I_DIRTY_PAGES set
> >   leaves __mark_inode_dirty()
> > 					clears I_DIRTY_PAGES
> > 					releases inode->i_lock
> >
> > Now @inode has dirty pages w/ I_DIRTY_PAGES clear.  This doesn't seem
> > to lead to an immediately critical problem because requeue_inode()
> > later checks PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES when
> > deciding whether the inode needs to be requeued for IO and there are
> > enough unintentional memory barriers inbetween, so while the inode
> > ends up with inconsistent I_DIRTY_PAGES flag, it doesn't fall off the
> > IO list.
> >
> > The lack of explicit barrier may also theoretically affect the other
> > I_DIRTY bits which deal with metadata dirtiness.  There is no
> > guarantee that a strong enough barrier exists between
> > I_DIRTY_[DATA]SYNC clearing and write_inode() writing out the dirtied
> > inode.  Filesystem inode writeout path likely has enough stuff which
> > can behave as full barrier but it's theoretically possible that the
> > writeout may not see all the updates from ->dirty_inode().
> >
> > Fix it by adding an explicit smp_mb() after I_DIRTY clearing.  Note
> > that I_DIRTY_PAGES needs a special treatment as it always needs to be
> > cleared to be interlocked with the lockless test on
> > __mark_inode_dirty() side.  It's cleared unconditionally and
> > reinstated after smp_mb() if the mapping still has dirty pages.
> >
> > Also add comments explaining how and why the barriers are paired.
> >
> > Lightly tested.
> >
> > Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Mikulas Patocka <mpatocka@xxxxxxxxxx>
> > Cc: Jens Axboe <axboe@xxxxxxxxx>
> > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
>
> Jens, can you please route this one?

I can, was going to send an ack anyway.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/