Re: [PATCH 2/4] writeback: Avoid skipping inode writeback

[Jan Kara <jack@suse.cz>]

On Fri 12-06-20 10:03:18, Martijn Coenen wrote:
> Hi Jan,
> 
> On Thu, Jun 11, 2020 at 10:12 AM Jan Kara <jack@suse.cz> wrote:
> > Reported-by: Martijn Coenen <maco@android.com>
> > Fixes: 0ae45f63d4ef ("vfs: add support for a lazytime mount option")
> > CC: stable@vger.kernel.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Thanks again for the fix. We've been running this (well, v1) for over
> 2 weeks across at least ~1000 Android devices with different kernel
> versions, and I can confirm we haven't run into the issue this intends
> to fix, or any other issues for that matter. The patch LGTM as well.
> 
> Reviewed-by: Martijn Coenen <maco@android.com>
> Tested-by: Martijn Coenen <maco@android.com>

Thanks for testing and review! I'll queue the patches in my tree and push
them to Linus later this week.

								Honza


> 
> > ---
> >  fs/fs-writeback.c  | 17 ++++++++++++-----
> >  include/linux/fs.h |  8 ++++++--
> >  2 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index ff0b18331590..f470c10641c5 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -146,6 +146,7 @@ static void inode_io_list_del_locked(struct inode *inode,
> >         assert_spin_locked(&wb->list_lock);
> >         assert_spin_locked(&inode->i_lock);
> >
> > +       inode->i_state &= ~I_SYNC_QUEUED;
> >         list_del_init(&inode->i_io_list);
> >         wb_io_lists_depopulated(wb);
> >  }
> > @@ -1187,6 +1188,7 @@ static void redirty_tail_locked(struct inode *inode, struct bdi_writeback *wb)
> >                         inode->dirtied_when = jiffies;
> >         }
> >         inode_io_list_move_locked(inode, wb, &wb->b_dirty);
> > +       inode->i_state &= ~I_SYNC_QUEUED;
> >  }
> >
> >  static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)
> > @@ -1262,8 +1264,11 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> >                         break;
> >                 list_move(&inode->i_io_list, &tmp);
> >                 moved++;
> > +               spin_lock(&inode->i_lock);
> >                 if (flags & EXPIRE_DIRTY_ATIME)
> > -                       set_bit(__I_DIRTY_TIME_EXPIRED, &inode->i_state);
> > +                       inode->i_state |= I_DIRTY_TIME_EXPIRED;
> > +               inode->i_state |= I_SYNC_QUEUED;
> > +               spin_unlock(&inode->i_lock);
> >                 if (sb_is_blkdev_sb(inode->i_sb))
> >                         continue;
> >                 if (sb && sb != inode->i_sb)
> > @@ -1438,6 +1443,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
> >         } else if (inode->i_state & I_DIRTY_TIME) {
> >                 inode->dirtied_when = jiffies;
> >                 inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> > +               inode->i_state &= ~I_SYNC_QUEUED;
> >         } else {
> >                 /* The inode is clean. Remove from writeback lists. */
> >                 inode_io_list_del_locked(inode, wb);
> > @@ -2301,11 +2307,12 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> >                 inode->i_state |= flags;
> >
> >                 /*
> > -                * If the inode is being synced, just update its dirty state.
> > -                * The unlocker will place the inode on the appropriate
> > -                * superblock list, based upon its state.
> > +                * If the inode is queued for writeback by flush worker, just
> > +                * update its dirty state. Once the flush worker is done with
> > +                * the inode it will place it on the appropriate superblock
> > +                * list, based upon its state.
> >                  */
> > -               if (inode->i_state & I_SYNC)
> > +               if (inode->i_state & I_SYNC_QUEUED)
> >                         goto out_unlock_inode;
> >
> >                 /*
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 19ef6c88c152..48556efcdcf0 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2157,6 +2157,10 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >   *
> >   * I_DONTCACHE         Evict inode as soon as it is not used anymore.
> >   *
> > + * I_SYNC_QUEUED       Inode is queued in b_io or b_more_io writeback lists.
> > + *                     Used to detect that mark_inode_dirty() should not move
> > + *                     inode between dirty lists.
> > + *
> >   * Q: What is the difference between I_WILL_FREE and I_FREEING?
> >   */
> >  #define I_DIRTY_SYNC           (1 << 0)
> > @@ -2174,12 +2178,12 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
> >  #define I_DIO_WAKEUP           (1 << __I_DIO_WAKEUP)
> >  #define I_LINKABLE             (1 << 10)
> >  #define I_DIRTY_TIME           (1 << 11)
> > -#define __I_DIRTY_TIME_EXPIRED 12
> > -#define I_DIRTY_TIME_EXPIRED   (1 << __I_DIRTY_TIME_EXPIRED)
> > +#define I_DIRTY_TIME_EXPIRED   (1 << 12)
> >  #define I_WB_SWITCH            (1 << 13)
> >  #define I_OVL_INUSE            (1 << 14)
> >  #define I_CREATING             (1 << 15)
> >  #define I_DONTCACHE            (1 << 16)
> > +#define I_SYNC_QUEUED          (1 << 17)
> >
> >  #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
> >  #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> > --
> > 2.16.4
> >
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR