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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 16 Oct 2019 08:08:13 -0400
From:   Brian Foster <bfoster@...hat.com>
To:     Christoph Hellwig <hch@....de>
Cc:     Dave Chinner <david@...morbit.com>,
        "Darrick J . Wong" <darrick.wong@...cle.com>,
        Damien Le Moal <Damien.LeMoal@....com>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/12] iomap: lift the xfs writeback code to iomap

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