| Message ID | 20200611081203.18161-4-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | writeback: Lazytime handling fix and cleanups | expand |
[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. --D > 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 >
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 > >
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"} \ )