Re: [PATCH 09/12] iomap: lift the xfs writeback code to iomap

[Brian Foster]

On Wed, Oct 16, 2019 at 09:48:36AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 09:07:21AM +1100, Dave Chinner wrote:
...
> > > +/*
> > > + * Submit the bio for an ioend. We are passed an ioend with a bio attached to
> > > + * it, and we submit that bio. The ioend may be used for multiple bio
> > > + * submissions, so we only want to allocate an append transaction for the ioend
> > > + * once.  In the case of multiple bio submission, each bio will take an IO
> > 
> > This needs to be changed to describe what wpc->ops->submit_ioend()
> > is used for rather than what XFS might use this hook for.
> 
> True.  The real documentation now is in the header near the ops defintion,
> but I'll update this one to make more sense as well.
> 
> > > +static int
> > > +iomap_submit_ioend(struct iomap_writepage_ctx *wpc, struct iomap_ioend *ioend,
> > > +		int error)
> > > +{
> > > +	ioend->io_bio->bi_private = ioend;
> > > +	ioend->io_bio->bi_end_io = iomap_writepage_end_bio;
> > > +
> > > +	if (wpc->ops->submit_ioend)
> > > +		error = wpc->ops->submit_ioend(ioend, error);
> > 
> > I'm not sure that "submit_ioend" is the best name for this method,
> > as it is a pre-bio-submission hook, not an actual IO submission
> > method. "prepare_ioend_for_submit" is more descriptive, but probably
> > too long. wpc->ops->prepare_submit(ioend, error) reads pretty well,
> > though...
> 
> Not a huge fan of that name either, but Brian complained.  Let's hold
> a popular vote for a name and see if we have a winner.
> 

Just to recall, I suggested something like ->pre_submit_ioend() back in
v5. Short of that, I asked for extra comments to clearly document
semantics which I believe Christoph added to the header, and I acked (it
looks like the handful of R-B tags I sent were all dropped btw...? Have
all of those patches changed?).

To give my .02 on the naming thing, I care about functional clarity more
than aesthetics. In that regard, ->submit_ioend() reads like a
submission hook and thus sounds rather confusing to me when I don't see
it actually submit anything. ->pre_submit_ioend(), ->prepare_ioend() or
->prepare_submission() all clearly indicate semantics to me, so I'm good
with any of those over ->submit_ioend(). :)

Brian

> As for the grammar comments - all this is copied over as-is.  I'll add
> another patch to fix that up.
>