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

[Brian Foster <bfoster@redhat.com>]

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.
> 

Ack.

> > 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.
> 
> > Another randomish idea might be to define a flag like
> > IOMAP_F_EOF_TRIMMED for ioends that are trimmed to EOF. Then perhaps we
> > could make an explicit decision not to grow or merge such ioends, and
> > let the associated code use io_size as is.
> 
> I don't think such a branch is any cheaper than the rounding..
> 

True, but I figured it to be more informational/usable than performance
oriented.

IOW following the train of thought in the other subthread, would any
practical workload be affected if we just trimmed io_size when needed by
i_size and left it at that?

If not, then you could use a flag to be more deliberate/informative that
the ioend was slightly special in that it was modified on submission,
and then adjacent ioends would simply fail to merge on a flag comparison
rather than fail to merge on a contiguity check.

Of course if folks would rather just do the rounding helper thing and
leave it up to the fs to use it, then I don't see any fundamental
problem with that. I just find it kind of a subtle/quirky interface.

Brian