[PATCH 0/3] Cleanup some writeback codes

[PATCH 0/3] Cleanup some writeback codes

[Tang Yizhou]

From: Tang Yizhou <yizhou.tang@shopee.com>

Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL and update some comments.

Tang Yizhou (3):
  mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL
  mm/page-writeback.c: Fix comment of wb_domain_writeout_add()
  xfs: Fix comment of xfs_buffered_write_iomap_begin()

 fs/xfs/xfs_iomap.c  |  2 +-
 mm/page-writeback.c | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Tang Yizhou]

From: Tang Yizhou <yizhou.tang@shopee.com>

The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
only used in the bandwidth update functions wb_update_bandwidth() and
__wb_update_bandwidth(), but also in the dirty limit update function
domain_update_dirty_limit().

Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.

This patche doesn't introduce any behavioral changes.

Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
---
 mm/page-writeback.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index fcd4c1439cb9..a848e7f0719d 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -54,9 +54,9 @@
 #define DIRTY_POLL_THRESH	(128 >> (PAGE_SHIFT - 10))
 
 /*
- * Estimate write bandwidth at 200ms intervals.
+ * Estimate write bandwidth or update dirty limit at 200ms intervals.
  */
-#define BANDWIDTH_INTERVAL	max(HZ/5, 1)
+#define UPDATE_INTERVAL		max(HZ/5, 1)
 
 #define RATELIMIT_CALC_SHIFT	10
 
@@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
 	/*
 	 * check locklessly first to optimize away locking for the most time
 	 */
-	if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
+	if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
 		return;
 
 	spin_lock(&dom->lock);
-	if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
+	if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
 		update_dirty_limit(dtc);
 		dom->dirty_limit_tstamp = now;
 	}
@@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
 		wb->dirty_exceeded = gdtc->dirty_exceeded ||
 				     (mdtc && mdtc->dirty_exceeded);
 		if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
-					   BANDWIDTH_INTERVAL))
+					   UPDATE_INTERVAL))
 			__wb_update_bandwidth(gdtc, mdtc, true);
 
 		/* throttle according to the chosen dtc */
@@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	 * writeback bandwidth is updated once in a while.
 	 */
 	if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
-				   BANDWIDTH_INTERVAL))
+				   UPDATE_INTERVAL))
 		wb_update_bandwidth(wb);
 	return ret;
 }
@@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
 	atomic_dec(&wb->writeback_inodes);
 	/*
 	 * Make sure estimate of writeback throughput gets updated after
-	 * writeback completed. We delay the update by BANDWIDTH_INTERVAL
+	 * writeback completed. We delay the update by UPDATE_INTERVAL
 	 * (which is the interval other bandwidth updates use for batching) so
 	 * that if multiple inodes end writeback at a similar time, they get
 	 * batched into one bandwidth update.
 	 */
 	spin_lock_irqsave(&wb->work_lock, flags);
 	if (test_bit(WB_registered, &wb->state))
-		queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
+		queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
 	spin_unlock_irqrestore(&wb->work_lock, flags);
 }
 
-- 
2.25.1

[PATCH 2/3] mm/page-writeback.c: Fix comment of wb_domain_writeout_add()

[Tang Yizhou]

From: Tang Yizhou <yizhou.tang@shopee.com>

__bdi_writeout_inc() has undergone multiple renamings, but the comment
within the function body have not been updated accordingly. Update it
to reflect the latest wb_domain_writeout_add().

Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
---
 mm/page-writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index a848e7f0719d..4f6efaa060bd 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -586,7 +586,7 @@ static void wb_domain_writeout_add(struct wb_domain *dom,
 	/* First event after period switching was turned off? */
 	if (unlikely(!dom->period_time)) {
 		/*
-		 * We can race with other __bdi_writeout_inc calls here but
+		 * We can race with other wb_domain_writeout_add calls here but
 		 * it does not cause any harm since the resulting time when
 		 * timer will fire and what is in writeout_period_time will be
 		 * roughly the same.
-- 
2.25.1

[PATCH 3/3] xfs: Fix comment of xfs_buffered_write_iomap_begin()

[Tang Yizhou]

From: Tang Yizhou <yizhou.tang@shopee.com>

Since macro MAX_WRITEBACK_PAGES has been removed from the writeback
path, change MAX_WRITEBACK_PAGES to the actual value of 1024.

Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
---
 fs/xfs/xfs_iomap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1e11f48814c0..bb4018395b6e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1097,7 +1097,7 @@ xfs_buffered_write_iomap_begin(
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 	} else {
 		/*
-		 * We cap the maximum length we map here to MAX_WRITEBACK_PAGES
+		 * We cap the maximum length we map here to 1024
 		 * pages to keep the chunks of work done where somewhat
 		 * symmetric with the work writeback does.  This is a completely
 		 * arbitrary number pulled out of thin air.
-- 
2.25.1

Re: [PATCH 3/3] xfs: Fix comment of xfs_buffered_write_iomap_begin()

[Christoph Hellwig]

On Wed, Oct 02, 2024 at 09:00:04PM +0800, Tang Yizhou wrote:
> From: Tang Yizhou <yizhou.tang@shopee.com>
> 
> Since macro MAX_WRITEBACK_PAGES has been removed from the writeback
> path, change MAX_WRITEBACK_PAGES to the actual value of 1024.

Well, that's an indicator that this code need a bit of a resync with
the writeback code so that the comment stays true.

Re: [PATCH 2/3] mm/page-writeback.c: Fix comment of wb_domain_writeout_add()

[Jan Kara]

On Wed 02-10-24 21:00:03, Tang Yizhou wrote:
> From: Tang Yizhou <yizhou.tang@shopee.com>
> 
> __bdi_writeout_inc() has undergone multiple renamings, but the comment
> within the function body have not been updated accordingly. Update it
> to reflect the latest wb_domain_writeout_add().
> 
> Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/page-writeback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index a848e7f0719d..4f6efaa060bd 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -586,7 +586,7 @@ static void wb_domain_writeout_add(struct wb_domain *dom,
>  	/* First event after period switching was turned off? */
>  	if (unlikely(!dom->period_time)) {
>  		/*
> -		 * We can race with other __bdi_writeout_inc calls here but
> +		 * We can race with other wb_domain_writeout_add calls here but
>  		 * it does not cause any harm since the resulting time when
>  		 * timer will fire and what is in writeout_period_time will be
>  		 * roughly the same.
> -- 
> 2.25.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Jan Kara]

On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> From: Tang Yizhou <yizhou.tang@shopee.com>
> 
> The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> only used in the bandwidth update functions wb_update_bandwidth() and
> __wb_update_bandwidth(), but also in the dirty limit update function
> domain_update_dirty_limit().
> 
> Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> 
> This patche doesn't introduce any behavioral changes.
> 
> Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>

Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
not seem much better to be honest. I actually have hard time coming up with
a more descriptive name so what if we settled on updating the comment only
instead of renaming to something not much better?

								Honza

> ---
>  mm/page-writeback.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index fcd4c1439cb9..a848e7f0719d 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -54,9 +54,9 @@
>  #define DIRTY_POLL_THRESH	(128 >> (PAGE_SHIFT - 10))
>  
>  /*
> - * Estimate write bandwidth at 200ms intervals.
> + * Estimate write bandwidth or update dirty limit at 200ms intervals.
>   */
> -#define BANDWIDTH_INTERVAL	max(HZ/5, 1)
> +#define UPDATE_INTERVAL		max(HZ/5, 1)
>  
>  #define RATELIMIT_CALC_SHIFT	10
>  
> @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
>  	/*
>  	 * check locklessly first to optimize away locking for the most time
>  	 */
> -	if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> +	if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
>  		return;
>  
>  	spin_lock(&dom->lock);
> -	if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> +	if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
>  		update_dirty_limit(dtc);
>  		dom->dirty_limit_tstamp = now;
>  	}
> @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  		wb->dirty_exceeded = gdtc->dirty_exceeded ||
>  				     (mdtc && mdtc->dirty_exceeded);
>  		if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> -					   BANDWIDTH_INTERVAL))
> +					   UPDATE_INTERVAL))
>  			__wb_update_bandwidth(gdtc, mdtc, true);
>  
>  		/* throttle according to the chosen dtc */
> @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  	 * writeback bandwidth is updated once in a while.
>  	 */
>  	if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> -				   BANDWIDTH_INTERVAL))
> +				   UPDATE_INTERVAL))
>  		wb_update_bandwidth(wb);
>  	return ret;
>  }
> @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
>  	atomic_dec(&wb->writeback_inodes);
>  	/*
>  	 * Make sure estimate of writeback throughput gets updated after
> -	 * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> +	 * writeback completed. We delay the update by UPDATE_INTERVAL
>  	 * (which is the interval other bandwidth updates use for batching) so
>  	 * that if multiple inodes end writeback at a similar time, they get
>  	 * batched into one bandwidth update.
>  	 */
>  	spin_lock_irqsave(&wb->work_lock, flags);
>  	if (test_bit(WB_registered, &wb->state))
> -		queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> +		queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
>  	spin_unlock_irqrestore(&wb->work_lock, flags);
>  }
>  
> -- 
> 2.25.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Tang Yizhou]

On Thu, Oct 3, 2024 at 9:01 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> > From: Tang Yizhou <yizhou.tang@shopee.com>
> >
> > The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> > only used in the bandwidth update functions wb_update_bandwidth() and
> > __wb_update_bandwidth(), but also in the dirty limit update function
> > domain_update_dirty_limit().
> >
> > Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> >
> > This patche doesn't introduce any behavioral changes.
> >
> > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
>
> Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
> not seem much better to be honest. I actually have hard time coming up with
> a more descriptive name so what if we settled on updating the comment only
> instead of renaming to something not much better?
>
>                                                                 Honza

Thank you for your review. I agree that UPDATE_INTERVAL is not a good
name. How about
renaming it to BW_DIRTYLIMIT_INTERVAL?

Yi

> > ---
> >  mm/page-writeback.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index fcd4c1439cb9..a848e7f0719d 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -54,9 +54,9 @@
> >  #define DIRTY_POLL_THRESH    (128 >> (PAGE_SHIFT - 10))
> >
> >  /*
> > - * Estimate write bandwidth at 200ms intervals.
> > + * Estimate write bandwidth or update dirty limit at 200ms intervals.
> >   */
> > -#define BANDWIDTH_INTERVAL   max(HZ/5, 1)
> > +#define UPDATE_INTERVAL              max(HZ/5, 1)
> >
> >  #define RATELIMIT_CALC_SHIFT 10
> >
> > @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
> >       /*
> >        * check locklessly first to optimize away locking for the most time
> >        */
> > -     if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> > +     if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
> >               return;
> >
> >       spin_lock(&dom->lock);
> > -     if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> > +     if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
> >               update_dirty_limit(dtc);
> >               dom->dirty_limit_tstamp = now;
> >       }
> > @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
> >               wb->dirty_exceeded = gdtc->dirty_exceeded ||
> >                                    (mdtc && mdtc->dirty_exceeded);
> >               if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > -                                        BANDWIDTH_INTERVAL))
> > +                                        UPDATE_INTERVAL))
> >                       __wb_update_bandwidth(gdtc, mdtc, true);
> >
> >               /* throttle according to the chosen dtc */
> > @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> >        * writeback bandwidth is updated once in a while.
> >        */
> >       if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > -                                BANDWIDTH_INTERVAL))
> > +                                UPDATE_INTERVAL))
> >               wb_update_bandwidth(wb);
> >       return ret;
> >  }
> > @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
> >       atomic_dec(&wb->writeback_inodes);
> >       /*
> >        * Make sure estimate of writeback throughput gets updated after
> > -      * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> > +      * writeback completed. We delay the update by UPDATE_INTERVAL
> >        * (which is the interval other bandwidth updates use for batching) so
> >        * that if multiple inodes end writeback at a similar time, they get
> >        * batched into one bandwidth update.
> >        */
> >       spin_lock_irqsave(&wb->work_lock, flags);
> >       if (test_bit(WB_registered, &wb->state))
> > -             queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> > +             queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
> >       spin_unlock_irqrestore(&wb->work_lock, flags);
> >  }
> >
> > --
> > 2.25.1
> >
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH 3/3] xfs: Fix comment of xfs_buffered_write_iomap_begin()

[Tang Yizhou]

On Wed, Oct 2, 2024 at 9:45 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Oct 02, 2024 at 09:00:04PM +0800, Tang Yizhou wrote:
> > From: Tang Yizhou <yizhou.tang@shopee.com>
> >
> > Since macro MAX_WRITEBACK_PAGES has been removed from the writeback
> > path, change MAX_WRITEBACK_PAGES to the actual value of 1024.
>
> Well, that's an indicator that this code need a bit of a resync with
> the writeback code so that the comment stays true.

Thanks for your advice. I will rewrite the code following the logic of
writeback_chunk_size().

Yi

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Jan Kara]

On Sun 06-10-24 20:41:11, Tang Yizhou wrote:
> On Thu, Oct 3, 2024 at 9:01 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> > > From: Tang Yizhou <yizhou.tang@shopee.com>
> > >
> > > The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> > > only used in the bandwidth update functions wb_update_bandwidth() and
> > > __wb_update_bandwidth(), but also in the dirty limit update function
> > > domain_update_dirty_limit().
> > >
> > > Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> > >
> > > This patche doesn't introduce any behavioral changes.
> > >
> > > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
> >
> > Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
> > not seem much better to be honest. I actually have hard time coming up with
> > a more descriptive name so what if we settled on updating the comment only
> > instead of renaming to something not much better?
> >
> >                                                                 Honza
> 
> Thank you for your review. I agree that UPDATE_INTERVAL is not a good
> name. How about
> renaming it to BW_DIRTYLIMIT_INTERVAL?

Maybe WB_STAT_INTERVAL? Because it is interval in which we maintain
statistics about writeback behavior.

								Honza

> > > ---
> > >  mm/page-writeback.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > index fcd4c1439cb9..a848e7f0719d 100644
> > > --- a/mm/page-writeback.c
> > > +++ b/mm/page-writeback.c
> > > @@ -54,9 +54,9 @@
> > >  #define DIRTY_POLL_THRESH    (128 >> (PAGE_SHIFT - 10))
> > >
> > >  /*
> > > - * Estimate write bandwidth at 200ms intervals.
> > > + * Estimate write bandwidth or update dirty limit at 200ms intervals.
> > >   */
> > > -#define BANDWIDTH_INTERVAL   max(HZ/5, 1)
> > > +#define UPDATE_INTERVAL              max(HZ/5, 1)
> > >
> > >  #define RATELIMIT_CALC_SHIFT 10
> > >
> > > @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
> > >       /*
> > >        * check locklessly first to optimize away locking for the most time
> > >        */
> > > -     if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> > > +     if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
> > >               return;
> > >
> > >       spin_lock(&dom->lock);
> > > -     if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> > > +     if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
> > >               update_dirty_limit(dtc);
> > >               dom->dirty_limit_tstamp = now;
> > >       }
> > > @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
> > >               wb->dirty_exceeded = gdtc->dirty_exceeded ||
> > >                                    (mdtc && mdtc->dirty_exceeded);
> > >               if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > -                                        BANDWIDTH_INTERVAL))
> > > +                                        UPDATE_INTERVAL))
> > >                       __wb_update_bandwidth(gdtc, mdtc, true);
> > >
> > >               /* throttle according to the chosen dtc */
> > > @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> > >        * writeback bandwidth is updated once in a while.
> > >        */
> > >       if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > -                                BANDWIDTH_INTERVAL))
> > > +                                UPDATE_INTERVAL))
> > >               wb_update_bandwidth(wb);
> > >       return ret;
> > >  }
> > > @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
> > >       atomic_dec(&wb->writeback_inodes);
> > >       /*
> > >        * Make sure estimate of writeback throughput gets updated after
> > > -      * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> > > +      * writeback completed. We delay the update by UPDATE_INTERVAL
> > >        * (which is the interval other bandwidth updates use for batching) so
> > >        * that if multiple inodes end writeback at a similar time, they get
> > >        * batched into one bandwidth update.
> > >        */
> > >       spin_lock_irqsave(&wb->work_lock, flags);
> > >       if (test_bit(WB_registered, &wb->state))
> > > -             queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> > > +             queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
> > >       spin_unlock_irqrestore(&wb->work_lock, flags);
> > >  }
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Tang Yizhou]

On Tue, Oct 8, 2024 at 12:23 AM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 06-10-24 20:41:11, Tang Yizhou wrote:
> > On Thu, Oct 3, 2024 at 9:01 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> > > > From: Tang Yizhou <yizhou.tang@shopee.com>
> > > >
> > > > The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> > > > only used in the bandwidth update functions wb_update_bandwidth() and
> > > > __wb_update_bandwidth(), but also in the dirty limit update function
> > > > domain_update_dirty_limit().
> > > >
> > > > Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> > > >
> > > > This patche doesn't introduce any behavioral changes.
> > > >
> > > > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
> > >
> > > Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
> > > not seem much better to be honest. I actually have hard time coming up with
> > > a more descriptive name so what if we settled on updating the comment only
> > > instead of renaming to something not much better?
> > >
> > >                                                                 Honza
> >
> > Thank you for your review. I agree that UPDATE_INTERVAL is not a good
> > name. How about
> > renaming it to BW_DIRTYLIMIT_INTERVAL?
>
> Maybe WB_STAT_INTERVAL? Because it is interval in which we maintain
> statistics about writeback behavior.
>

I don't think this is a good name, as it suggests a relation to enum
wb_stat_item, but bandwidth and dirty limit are not in wb_stat_item.

Yi

>                                                                 Honza
>
> > > > ---
> > > >  mm/page-writeback.c | 16 ++++++++--------
> > > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > index fcd4c1439cb9..a848e7f0719d 100644
> > > > --- a/mm/page-writeback.c
> > > > +++ b/mm/page-writeback.c
> > > > @@ -54,9 +54,9 @@
> > > >  #define DIRTY_POLL_THRESH    (128 >> (PAGE_SHIFT - 10))
> > > >
> > > >  /*
> > > > - * Estimate write bandwidth at 200ms intervals.
> > > > + * Estimate write bandwidth or update dirty limit at 200ms intervals.
> > > >   */
> > > > -#define BANDWIDTH_INTERVAL   max(HZ/5, 1)
> > > > +#define UPDATE_INTERVAL              max(HZ/5, 1)
> > > >
> > > >  #define RATELIMIT_CALC_SHIFT 10
> > > >
> > > > @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
> > > >       /*
> > > >        * check locklessly first to optimize away locking for the most time
> > > >        */
> > > > -     if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> > > > +     if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
> > > >               return;
> > > >
> > > >       spin_lock(&dom->lock);
> > > > -     if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> > > > +     if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
> > > >               update_dirty_limit(dtc);
> > > >               dom->dirty_limit_tstamp = now;
> > > >       }
> > > > @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
> > > >               wb->dirty_exceeded = gdtc->dirty_exceeded ||
> > > >                                    (mdtc && mdtc->dirty_exceeded);
> > > >               if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > -                                        BANDWIDTH_INTERVAL))
> > > > +                                        UPDATE_INTERVAL))
> > > >                       __wb_update_bandwidth(gdtc, mdtc, true);
> > > >
> > > >               /* throttle according to the chosen dtc */
> > > > @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> > > >        * writeback bandwidth is updated once in a while.
> > > >        */
> > > >       if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > -                                BANDWIDTH_INTERVAL))
> > > > +                                UPDATE_INTERVAL))
> > > >               wb_update_bandwidth(wb);
> > > >       return ret;
> > > >  }
> > > > @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
> > > >       atomic_dec(&wb->writeback_inodes);
> > > >       /*
> > > >        * Make sure estimate of writeback throughput gets updated after
> > > > -      * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> > > > +      * writeback completed. We delay the update by UPDATE_INTERVAL
> > > >        * (which is the interval other bandwidth updates use for batching) so
> > > >        * that if multiple inodes end writeback at a similar time, they get
> > > >        * batched into one bandwidth update.
> > > >        */
> > > >       spin_lock_irqsave(&wb->work_lock, flags);
> > > >       if (test_bit(WB_registered, &wb->state))
> > > > -             queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> > > > +             queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
> > > >       spin_unlock_irqrestore(&wb->work_lock, flags);
> > > >  }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > > --
> > > Jan Kara <jack@suse.com>
> > > SUSE Labs, CR
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Jan Kara]

On Tue 08-10-24 22:14:16, Tang Yizhou wrote:
> On Tue, Oct 8, 2024 at 12:23 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sun 06-10-24 20:41:11, Tang Yizhou wrote:
> > > On Thu, Oct 3, 2024 at 9:01 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> > > > > From: Tang Yizhou <yizhou.tang@shopee.com>
> > > > >
> > > > > The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> > > > > only used in the bandwidth update functions wb_update_bandwidth() and
> > > > > __wb_update_bandwidth(), but also in the dirty limit update function
> > > > > domain_update_dirty_limit().
> > > > >
> > > > > Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> > > > >
> > > > > This patche doesn't introduce any behavioral changes.
> > > > >
> > > > > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
> > > >
> > > > Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
> > > > not seem much better to be honest. I actually have hard time coming up with
> > > > a more descriptive name so what if we settled on updating the comment only
> > > > instead of renaming to something not much better?
> > > >
> > > >                                                                 Honza
> > >
> > > Thank you for your review. I agree that UPDATE_INTERVAL is not a good
> > > name. How about
> > > renaming it to BW_DIRTYLIMIT_INTERVAL?
> >
> > Maybe WB_STAT_INTERVAL? Because it is interval in which we maintain
> > statistics about writeback behavior.
> >
> 
> I don't think this is a good name, as it suggests a relation to enum
> wb_stat_item, but bandwidth and dirty limit are not in wb_stat_item.

OK, so how about keeping BANDWIDTH_INTERVAL as is and adding
DIRTY_LIMIT_INTERVAL with the same value? There's nothing which would
strictly tie them to the same value.

								Honza

> > > > > ---
> > > > >  mm/page-writeback.c | 16 ++++++++--------
> > > > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > > index fcd4c1439cb9..a848e7f0719d 100644
> > > > > --- a/mm/page-writeback.c
> > > > > +++ b/mm/page-writeback.c
> > > > > @@ -54,9 +54,9 @@
> > > > >  #define DIRTY_POLL_THRESH    (128 >> (PAGE_SHIFT - 10))
> > > > >
> > > > >  /*
> > > > > - * Estimate write bandwidth at 200ms intervals.
> > > > > + * Estimate write bandwidth or update dirty limit at 200ms intervals.
> > > > >   */
> > > > > -#define BANDWIDTH_INTERVAL   max(HZ/5, 1)
> > > > > +#define UPDATE_INTERVAL              max(HZ/5, 1)
> > > > >
> > > > >  #define RATELIMIT_CALC_SHIFT 10
> > > > >
> > > > > @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
> > > > >       /*
> > > > >        * check locklessly first to optimize away locking for the most time
> > > > >        */
> > > > > -     if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> > > > > +     if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
> > > > >               return;
> > > > >
> > > > >       spin_lock(&dom->lock);
> > > > > -     if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> > > > > +     if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
> > > > >               update_dirty_limit(dtc);
> > > > >               dom->dirty_limit_tstamp = now;
> > > > >       }
> > > > > @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
> > > > >               wb->dirty_exceeded = gdtc->dirty_exceeded ||
> > > > >                                    (mdtc && mdtc->dirty_exceeded);
> > > > >               if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > > -                                        BANDWIDTH_INTERVAL))
> > > > > +                                        UPDATE_INTERVAL))
> > > > >                       __wb_update_bandwidth(gdtc, mdtc, true);
> > > > >
> > > > >               /* throttle according to the chosen dtc */
> > > > > @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> > > > >        * writeback bandwidth is updated once in a while.
> > > > >        */
> > > > >       if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > > -                                BANDWIDTH_INTERVAL))
> > > > > +                                UPDATE_INTERVAL))
> > > > >               wb_update_bandwidth(wb);
> > > > >       return ret;
> > > > >  }
> > > > > @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
> > > > >       atomic_dec(&wb->writeback_inodes);
> > > > >       /*
> > > > >        * Make sure estimate of writeback throughput gets updated after
> > > > > -      * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> > > > > +      * writeback completed. We delay the update by UPDATE_INTERVAL
> > > > >        * (which is the interval other bandwidth updates use for batching) so
> > > > >        * that if multiple inodes end writeback at a similar time, they get
> > > > >        * batched into one bandwidth update.
> > > > >        */
> > > > >       spin_lock_irqsave(&wb->work_lock, flags);
> > > > >       if (test_bit(WB_registered, &wb->state))
> > > > > -             queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> > > > > +             queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
> > > > >       spin_unlock_irqrestore(&wb->work_lock, flags);
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > >
> > > > --
> > > > Jan Kara <jack@suse.com>
> > > > SUSE Labs, CR
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] mm/page-writeback.c: Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL

[Tang Yizhou]

On Wed, Oct 9, 2024 at 10:55 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 08-10-24 22:14:16, Tang Yizhou wrote:
> > On Tue, Oct 8, 2024 at 12:23 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Sun 06-10-24 20:41:11, Tang Yizhou wrote:
> > > > On Thu, Oct 3, 2024 at 9:01 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Wed 02-10-24 21:00:02, Tang Yizhou wrote:
> > > > > > From: Tang Yizhou <yizhou.tang@shopee.com>
> > > > > >
> > > > > > The name of the BANDWIDTH_INTERVAL macro is misleading, as it is not
> > > > > > only used in the bandwidth update functions wb_update_bandwidth() and
> > > > > > __wb_update_bandwidth(), but also in the dirty limit update function
> > > > > > domain_update_dirty_limit().
> > > > > >
> > > > > > Rename BANDWIDTH_INTERVAL to UPDATE_INTERVAL to make things clear.
> > > > > >
> > > > > > This patche doesn't introduce any behavioral changes.
> > > > > >
> > > > > > Signed-off-by: Tang Yizhou <yizhou.tang@shopee.com>
> > > > >
> > > > > Umm, I agree BANDWIDTH_INTERVAL may be confusing but UPDATE_INTERVAL does
> > > > > not seem much better to be honest. I actually have hard time coming up with
> > > > > a more descriptive name so what if we settled on updating the comment only
> > > > > instead of renaming to something not much better?
> > > > >
> > > > >                                                                 Honza
> > > >
> > > > Thank you for your review. I agree that UPDATE_INTERVAL is not a good
> > > > name. How about
> > > > renaming it to BW_DIRTYLIMIT_INTERVAL?
> > >
> > > Maybe WB_STAT_INTERVAL? Because it is interval in which we maintain
> > > statistics about writeback behavior.
> > >
> >
> > I don't think this is a good name, as it suggests a relation to enum
> > wb_stat_item, but bandwidth and dirty limit are not in wb_stat_item.
>
> OK, so how about keeping BANDWIDTH_INTERVAL as is and adding
> DIRTY_LIMIT_INTERVAL with the same value? There's nothing which would
> strictly tie them to the same value.
>

Good idea, but this patch has already been merged. If there is any
writeback-related code that needs to be modified next time, I will
update this part as well.

Yi

>                                                                 Honza
>
> > > > > > ---
> > > > > >  mm/page-writeback.c | 16 ++++++++--------
> > > > > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > > > index fcd4c1439cb9..a848e7f0719d 100644
> > > > > > --- a/mm/page-writeback.c
> > > > > > +++ b/mm/page-writeback.c
> > > > > > @@ -54,9 +54,9 @@
> > > > > >  #define DIRTY_POLL_THRESH    (128 >> (PAGE_SHIFT - 10))
> > > > > >
> > > > > >  /*
> > > > > > - * Estimate write bandwidth at 200ms intervals.
> > > > > > + * Estimate write bandwidth or update dirty limit at 200ms intervals.
> > > > > >   */
> > > > > > -#define BANDWIDTH_INTERVAL   max(HZ/5, 1)
> > > > > > +#define UPDATE_INTERVAL              max(HZ/5, 1)
> > > > > >
> > > > > >  #define RATELIMIT_CALC_SHIFT 10
> > > > > >
> > > > > > @@ -1331,11 +1331,11 @@ static void domain_update_dirty_limit(struct dirty_throttle_control *dtc,
> > > > > >       /*
> > > > > >        * check locklessly first to optimize away locking for the most time
> > > > > >        */
> > > > > > -     if (time_before(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL))
> > > > > > +     if (time_before(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL))
> > > > > >               return;
> > > > > >
> > > > > >       spin_lock(&dom->lock);
> > > > > > -     if (time_after_eq(now, dom->dirty_limit_tstamp + BANDWIDTH_INTERVAL)) {
> > > > > > +     if (time_after_eq(now, dom->dirty_limit_tstamp + UPDATE_INTERVAL)) {
> > > > > >               update_dirty_limit(dtc);
> > > > > >               dom->dirty_limit_tstamp = now;
> > > > > >       }
> > > > > > @@ -1928,7 +1928,7 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
> > > > > >               wb->dirty_exceeded = gdtc->dirty_exceeded ||
> > > > > >                                    (mdtc && mdtc->dirty_exceeded);
> > > > > >               if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > > > -                                        BANDWIDTH_INTERVAL))
> > > > > > +                                        UPDATE_INTERVAL))
> > > > > >                       __wb_update_bandwidth(gdtc, mdtc, true);
> > > > > >
> > > > > >               /* throttle according to the chosen dtc */
> > > > > > @@ -2705,7 +2705,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> > > > > >        * writeback bandwidth is updated once in a while.
> > > > > >        */
> > > > > >       if (time_is_before_jiffies(READ_ONCE(wb->bw_time_stamp) +
> > > > > > -                                BANDWIDTH_INTERVAL))
> > > > > > +                                UPDATE_INTERVAL))
> > > > > >               wb_update_bandwidth(wb);
> > > > > >       return ret;
> > > > > >  }
> > > > > > @@ -3057,14 +3057,14 @@ static void wb_inode_writeback_end(struct bdi_writeback *wb)
> > > > > >       atomic_dec(&wb->writeback_inodes);
> > > > > >       /*
> > > > > >        * Make sure estimate of writeback throughput gets updated after
> > > > > > -      * writeback completed. We delay the update by BANDWIDTH_INTERVAL
> > > > > > +      * writeback completed. We delay the update by UPDATE_INTERVAL
> > > > > >        * (which is the interval other bandwidth updates use for batching) so
> > > > > >        * that if multiple inodes end writeback at a similar time, they get
> > > > > >        * batched into one bandwidth update.
> > > > > >        */
> > > > > >       spin_lock_irqsave(&wb->work_lock, flags);
> > > > > >       if (test_bit(WB_registered, &wb->state))
> > > > > > -             queue_delayed_work(bdi_wq, &wb->bw_dwork, BANDWIDTH_INTERVAL);
> > > > > > +             queue_delayed_work(bdi_wq, &wb->bw_dwork, UPDATE_INTERVAL);
> > > > > >       spin_unlock_irqrestore(&wb->work_lock, flags);
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > > >
> > > > > --
> > > > > Jan Kara <jack@suse.com>
> > > > > SUSE Labs, CR
> > > --
> > > Jan Kara <jack@suse.com>
> > > SUSE Labs, CR
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR