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

[Brian Foster <bfoster@redhat.com>]

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.

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?

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?

Brian

> Thanks,
> Long Li
>  
>