Re: [PATCH 4/4] writeback: Drop I_DIRTY_TIME_EXPIRE

[Jan Kara <jack@suse.cz>]

On Wed 02-09-20 10:20:48, Darrick J. Wong wrote:
> [add linux-xfs and xfsprogs maintainer to cc]
> 
> On Thu, Jun 11, 2020 at 10:11:55AM +0200, Jan Kara wrote:
> > The only use of I_DIRTY_TIME_EXPIRE is to detect in
> > __writeback_single_inode() that inode got there because flush worker
> > decided it's time to writeback the dirty inode time stamps (either
> > because we are syncing or because of age). However we can detect this
> > directly in __writeback_single_inode() and there's no need for the
> > strange propagation with I_DIRTY_TIME_EXPIRE flag.
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext4/inode.c                  |  2 +-
> >  fs/fs-writeback.c                | 28 +++++++++++-----------------
> >  fs/xfs/libxfs/xfs_trans_inode.c  |  4 ++--
> 
> Urrk, so I only just noticed this when I rebased my development tree
> onto 5.9-rc3.  If you're going to change things in fs/xfs/, please cc
> the xfs list to keep us in the loop.  Changes to fs/xfs/libxfs/ have to
> be ported to userspace.

OK, will do next time. I was just dropping a generic flag XFS didn't use in
any particular way so it didn't occur to me XFS people would be interested...

								Honza

> >  include/linux/fs.h               |  1 -
> >  include/trace/events/writeback.h |  1 -
> >  5 files changed, 14 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 40ec5c7ef0d3..4db497f02ffb 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -4887,7 +4887,7 @@ static void __ext4_update_other_inode_time(struct super_block *sb,
> >  	    (inode->i_state & I_DIRTY_TIME)) {
> >  		struct ext4_inode_info	*ei = EXT4_I(inode);
> >  
> > -		inode->i_state &= ~(I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED);
> > +		inode->i_state &= ~I_DIRTY_TIME;
> >  		spin_unlock(&inode->i_lock);
> >  
> >  		spin_lock(&ei->i_raw_lock);
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index ae17d64a3e18..149227160ff0 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -1238,7 +1238,7 @@ static bool inode_dirtied_after(struct inode *inode, unsigned long t)
> >   */
> >  static int move_expired_inodes(struct list_head *delaying_queue,
> >  			       struct list_head *dispatch_queue,
> > -			       int flags, unsigned long dirtied_before)
> > +			       unsigned long dirtied_before)
> >  {
> >  	LIST_HEAD(tmp);
> >  	struct list_head *pos, *node;
> > @@ -1254,8 +1254,6 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> >  		list_move(&inode->i_io_list, &tmp);
> >  		moved++;
> >  		spin_lock(&inode->i_lock);
> > -		if (flags & EXPIRE_DIRTY_ATIME)
> > -			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))
> > @@ -1303,11 +1301,11 @@ static void queue_io(struct bdi_writeback *wb, struct wb_writeback_work *work,
> >  
> >  	assert_spin_locked(&wb->list_lock);
> >  	list_splice_init(&wb->b_more_io, &wb->b_io);
> > -	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, 0, dirtied_before);
> > +	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, dirtied_before);
> >  	if (!work->for_sync)
> >  		time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
> >  	moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
> > -				     EXPIRE_DIRTY_ATIME, time_expire_jif);
> > +				     time_expire_jif);
> >  	if (moved)
> >  		wb_io_lists_populated(wb);
> >  	trace_writeback_queue_io(wb, work, dirtied_before, moved);
> > @@ -1483,18 +1481,14 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> >  	spin_lock(&inode->i_lock);
> >  
> >  	dirty = inode->i_state & I_DIRTY;
> > -	if (inode->i_state & I_DIRTY_TIME) {
> > -		if ((dirty & I_DIRTY_INODE) ||
> > -		    wbc->sync_mode == WB_SYNC_ALL ||
> > -		    unlikely(inode->i_state & I_DIRTY_TIME_EXPIRED) ||
> > -		    unlikely(time_after(jiffies,
> > -					(inode->dirtied_time_when +
> > -					 dirtytime_expire_interval * HZ)))) {
> > -			dirty |= I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED;
> > -			trace_writeback_lazytime(inode);
> > -		}
> > -	} else
> > -		inode->i_state &= ~I_DIRTY_TIME_EXPIRED;
> > +	if ((inode->i_state & I_DIRTY_TIME) &&
> > +	    ((dirty & I_DIRTY_INODE) ||
> > +	     wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
> > +	     time_after(jiffies, inode->dirtied_time_when +
> > +			dirtytime_expire_interval * HZ))) {
> > +		dirty |= I_DIRTY_TIME;
> > +		trace_writeback_lazytime(inode);
> > +	}
> >  	inode->i_state &= ~dirty;
> >  
> >  	/*
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index b5dfb6654842..1b4df6636944 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -96,9 +96,9 @@ xfs_trans_log_inode(
> >  	 * to log the timestamps, or will clear already cleared fields in the
> >  	 * worst case.
> >  	 */
> > -	if (inode->i_state & (I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED)) {
> > +	if (inode->i_state & I_DIRTY_TIME) {
> >  		spin_lock(&inode->i_lock);
> > -		inode->i_state &= ~(I_DIRTY_TIME | I_DIRTY_TIME_EXPIRED);
> > +		inode->i_state &= ~I_DIRTY_TIME;
> >  		spin_unlock(&inode->i_lock);
> >  	}
> >  
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 48556efcdcf0..45eadf5bea5d 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2178,7 +2178,6 @@ 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	(1 << 12)
> >  #define I_WB_SWITCH		(1 << 13)
> >  #define I_OVL_INUSE		(1 << 14)
> >  #define I_CREATING		(1 << 15)
> > diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> > index 7565dcd59697..e7cbccc7c14c 100644
> > --- a/include/trace/events/writeback.h
> > +++ b/include/trace/events/writeback.h
> > @@ -20,7 +20,6 @@
> >  		{I_CLEAR,		"I_CLEAR"},		\
> >  		{I_SYNC,		"I_SYNC"},		\
> >  		{I_DIRTY_TIME,		"I_DIRTY_TIME"},	\
> > -		{I_DIRTY_TIME_EXPIRED,	"I_DIRTY_TIME_EXPIRED"}, \
> >  		{I_REFERENCED,		"I_REFERENCED"}		\
> >  	)
> >  
> > -- 
> > 2.16.4
> > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR