lkml.org 
[lkml]   [2010]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 7/7] fs: fix or note I_DIRTY handling bugs in filesystems
    On Wed, Nov 24, 2010 at 01:06:17AM +1100, npiggin@kernel.dk wrote:
    > Comments?

    How did you test the changes?

    > +++ linux-2.6/fs/xfs/linux-2.6/xfs_file.c 2010-11-24 00:08:03.000000000 +1100
    > @@ -99,6 +99,7 @@ xfs_file_fsync(
    > struct xfs_trans *tp;
    > int error = 0;
    > int log_flushed = 0;
    > + unsigned dirty, mask;
    >
    > trace_xfs_file_fsync(ip);
    >
    > @@ -132,9 +133,16 @@ xfs_file_fsync(
    > * might gets cleared when the inode gets written out via the AIL
    > * or xfs_iflush_cluster.
    > */
    > - if (((inode->i_state & I_DIRTY_DATASYNC) ||
    > - ((inode->i_state & I_DIRTY_SYNC) && !datasync)) &&
    > - ip->i_update_core) {
    > + spin_lock(&inode_lock);
    > + inode_writeback_begin(inode, 1);
    > + if (datasync)
    > + mask = I_DIRTY_DATASYNC;
    > + else
    > + mask = I_DIRTY_SYNC | I_DIRTY_DATASYNC;
    > + dirty = inode->i_state & mask;
    > + inode->i_state &= ~mask;
    > + spin_unlock(&inode_lock);
    > + if (dirty && ip->i_update_core) {

    It looks to me like the pattern "inode_writeback_begin(); get dirty
    state from i_state" repeated for each filesystem is wrong. The
    inode_writeback_begin() helper does this:

    inode->i_state &= ~I_DIRTY;

    which clears all the dirty bits from the i_state, which means the
    followup:

    dirty = inode->i_state & mask;

    will always result in a zero value for dirty. IOWs, this seems to
    ensure that ->fsync never sees dirty inodes anymore. This will break
    fsync on XFS, and probably on all the other filesystems you modified
    to use this pattern as well.

    Also, I think the pattern is racy with respect to concurrent page
    cache dirtiers. i.e if the inode was dirtied between writeback and
    ->fsync() in vfs_fsync_range(), then this new code clears the
    I_DIRTY_PAGES bit in i_state without writing back the dirty pages.

    And FWIW, I'm not sure that we want to be propagating the inode_lock
    into every filesystem...

    Cheers,

    Dave.
    --
    Dave Chinner
    david@fromorbit.com


    \
     
     \ /
      Last update: 2010-11-23 23:55    [W:0.023 / U:58.104 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site