Re: [PATCH v3 3/3] btrfs: rename struct btrfs_io_bio to btrfs_logical_bio

David Sterba
2021-09-20T12:23:46Z

On Mon, Sep 20, 2021 at 10:04:10AM +0300, Nikolay Borisov wrote:
>
>
> On 15.09.21 г. 10:17, Qu Wenruo wrote:
> > Previously we have "struct btrfs_bio", which records IO context for
> > mirrored IO and RAID56, and "strcut btrfs_io_bio", which records extra
> > btrfs specific info for logical bytenr bio.
> >
> > With "strcut btrfs_bio" renamed to "struct btrfs_io_context", we are
> > safe to rename "strcut btrfs_io_bio" to "strcut btrfs_logical_bio" which
> > is a more suitable name now.
> >
> > Although the name, "btrfs_logical_bio", is a little long and name
> > "btrfs_bio" can be much shorter, "btrfs_bio" conflicts with previous
> > "btrfs_bio" structure and can cause a lot of problems for backports.
> >
> > Thus here we choose the name "btrfs_logical_bio", which also emphasis
> > those bios all work at logical bytenr.
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> So thinking a bit more about the renaming we are trading "awkwardness"
> for future generations so that we make backporting easier or rather more
> fool proof.
>
> What if we backport a patch that does BUILD_BUG_ON predicated on the
> size of the btrfs_io_bio. That way if a patch backports cleanly and
> automatically but in fact git got confused by btrfs_bio vs btrfs_io_bio
> then a build failure would ensue due to mismatched sizes and that would
> be a clear indication something has gone wrong so whoever is doing the
> backport can go and correct the backport? David what do you think about
> this?

So you want to call the structure btrfs_bio and add build protections? I'm not
sure how exactly you want to do the sizeof check, one way would be to add a
stub structure and compare sizeof against that, because a hardcoded value won't
work due to padding, or we'd have to have a 32bit assertion version.

I'd like to see the code, but otherwise I think it's reasonable, the shorter
name would be better. I don't expect many backports regarding the bio
related code, it could be referenced in the diff context but that we can
handle fine. I'm a bit cautious because I've seen patches to other
subystems that did changes like swapping parameters or repurposing
structures like here we do and Linus did not like that at all. It's
trade off if we'll suffer a naming we don't like or would cause a bug
because we'd forget about the change.