* Re: [PATCH 4/4] writeback: Drop I_DIRTY_TIME_EXPIRE
2020-09-02 17:20 ` [PATCH 4/4] writeback: Drop I_DIRTY_TIME_EXPIRE Darrick J. Wong
@ 2020-09-03 10:10 ` Jan Kara
0 siblings, 0 replies; 2+ messages in thread
From: Jan Kara @ 2020-09-03 10:10 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Jan Kara, linux-fsdevel, Christoph Hellwig, Martijn Coenen, xfs,
Eric Sandeen
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
^ permalink raw reply [flat|nested] 2+ messages in thread