Re: [PATCH v2] xfs: remove redundant batch variables for serialization

[alexjlzheng]

On Mon, 11 Sep 2023 07:49:51 +1000, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Sat, Sep 09, 2023 at 03:17:51PM +0800, alexjlzheng@xxxxxxxxx wrote:
> > From: Jinliang Zheng <alexjlzheng@xxxxxxxxxxx>
> > 
> > Historically, when generic percpu counters were introduced in xfs for
> > free block counters by commit 0d485ada404b ("xfs: use generic percpu
> > counters for free block counter"), the counters used a custom batch
> > size. In xfs_mod_freecounter(), originally named xfs_mod_fdblocks(),
> > this patch attempted to serialize the program using a smaller batch size
> > as parameter to the addition function as the counter approaches 0.
> > 
> > Commit 8c1903d3081a ("xfs: inode and free block counters need to use
> > __percpu_counter_compare") pointed out the error in commit 0d485ada404b
> > ("xfs: use generic percpu counters for free block counter") mentioned
> > above and said that "Because the counters use a custom batch size, the
> > comparison functions need to be aware of that batch size otherwise the
> > comparison does not work correctly". Then percpu_counter_compare() was
> > replaced with __percpu_counter_compare() with parameter
> > XFS_FDBLOCKS_BATCH.
> > 
> > After commit 8c1903d3081a ("xfs: inode and free block counters need to
> > use __percpu_counter_compare"), the existence of the batch variable is
> > no longer necessary, so this patch is proposed to simplify the code by
> > removing it.
> 
> Hmmmm. Fiddling with percpu counter batch thresholds can expose
> unexpected corner case behaviours.  What testing have you done on
> this change?

Hi, Dave,

Thank you for your reply.

I have tested the patch using _filebench_ and _fio_.

In fact, corner cases often occur when the addition function and comparison
function use different batch thresholds. This is why the batch parameter of
the addition function must be chosen carefully when we use a additon function
with a variable parameter *percpu_counter_batch*.

The percpu_counter_compare() with variable parameter *percpu_counter_batch*
was replaced with __percpu_counter_compare() with fixed parameter
*XFS_FDBLOCKS_BATCH* by commit 8c1903d3081a ("xfs: inode and free block
counters need to use __percpu_counter_compare").

Then there is no need to detect the batch threshold. Just use the fixed
*XFS_FDBLOCKS_BATCH* to simplify the code.

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx

Best regards，

Jinliang Zheng.