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

[Martijn Coenen <maco@android.com>]

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>

> ---
>  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
>