Re: [PATCH v2 1/2] iomap: fix zero padding data issue in concurrent append writes

[Long Li <leo.lilong@huawei.com>]

On Tue, Nov 19, 2024 at 07:13:59AM -0500, Brian Foster wrote:
> On Tue, Nov 19, 2024 at 04:35:22PM +0800, Long Li wrote:
> > On Sun, Nov 17, 2024 at 10:56:59PM -0800, Christoph Hellwig wrote:
> > > On Wed, Nov 13, 2024 at 11:13:49AM -0500, Brian Foster wrote:
> > > > >  static bool
> > > > >  iomap_ioend_can_merge(struct iomap_ioend *ioend, struct iomap_ioend *next)
> > > > >  {
> > > > > +	size_t size = iomap_ioend_extent_size(ioend);
> > > > > +
> > > > 
> > > > The function name is kind of misleading IMO because this may not
> > > > necessarily reflect "extent size." Maybe something like
> > > > _ioend_size_aligned() would be more accurate..?
> > > 
> > > Agreed.  What also would be useful is a comment describing the
> > > function and why io_size is not aligned.
> > > 
> > 
> > Ok, it will be changed in the next version.
> > 
> > > > 1. It kind of feels like a landmine in an area where block alignment is
> > > > typically expected. I wonder if a rename to something like io_bytes
> > > > would help at all with that.
> > > 
> > > Fine with me.
> > > 
> > 
> > While continuing to use io_size may introduce some ambiguity, this can
> > be adequately addressed through proper documentation. Furthermore,
> > retaining io_size would minimize code changes. I would like to
> > confirm whether renaming io_size to io_bytes is truly necessary in
> > this context.
> > 
> 
> I don't think a rename is a requirement. It was just an idea to
> consider.
> 

ok.

> The whole rounding thing is the one lingering thing for me. In my mind
> it's not worth the complexity of having a special wrapper like this if
> we don't have at least one example where it provides tangible
> performance benefit. It kind of sounds like we're fishing around for
> examples where it would allow an ioend to merge, but so far don't have
> anything that reproduces perf. value. Do you agree with that assessment?
> 

Yes, I agree with your assessment. The merging through size rounding
actually occurs in only a small number of cases.

> That said, I agree we have a couple examples where it is technically
> functional, it does preserve existing logic, and it's not the biggest
> deal in general. So if we really want to keep it, perhaps a reasonable
> compromise might be to lift it as a static into buffered-io.c (so it's
> not exposed to new users via the header, at least for now) and add a
> nice comment above it to explain when/why the io_size is inferred via
> rounding and that it's specifically for ioend grow/merge management. Hm?
> 

I agree with you, this approach sounds reasonable to me.

Thanks,
Long Li