Re: [PATCH v2 00/29] xfsprogs: Drop the 'platform_' prefix

[Dave Chinner <david@fromorbit.com>]

On Sat, Aug 07, 2021 at 05:13:33PM +0200, Pavel Reichl wrote:
> 
> On 8/7/21 1:05 AM, Dave Chinner wrote:
> > On Fri, Aug 06, 2021 at 11:22:49PM +0200, Pavel Reichl wrote:
> > > Stop using commands with 'platform_' prefix. Either use directly linux
> > > standard command or drop the prefix from the function name.
> > Looks like all of the patches in this series are missing
> > signed-off-by lines.  Most of them have empty commit messages, too,
> Sorry about the missing signed-off-by, I really need to have a
> check-list before posting patches...or send more patches :-).

Having a checklist is a good idea. :)

> > which we don't tend to do very often.
> > 
> > >   51 files changed, 284 insertions(+), 151 deletions(-)
> > IMO, 29 patches for such a small change is way too fine grained for
> > working through efficiently.  Empty commit messages tend to be a
> > sign that you can aggregate patches together.... i.e.  One patch for
> > all the uuid changes (currently 7 patches) with a description of why
> > you're changing the platform_uuid interface, one for all the mount
> > related stuff (at least 5 patches now), one for all the block device
> > stuff (8 or so patches), one for all the path bits, and then one for
> > whatever is left over.
> OK, I'll do this in the next version.
> > 
> > Every patch has overhead, be it to produce, maintain, review, test,
> > merge, etc. Breaking stuff down unnecessarily just increases the
> > amount of work everyone has to do at every step. So if you find that
> > you are writing dozens of patches that each have a trivial change in
> > them that you are boiler-plating commit messages, you've probably
> > made the overall changeset too fine grained.
> OK, sincerely thank you for the 'rules-of-thump'. However, In the
> first version of the patch set I grouped the changes into way less patches
> and passed along a question about the preferred granularity of patches
> and got the following answer:
> 
> >   * What would be best for the reviewer - should I prepare a separate
> >   patch for every function rename or should I squash the changes into
> >   one huge patch?
> > One patch per function, please.
> 
> However, I agree that I should have mentioned that some patches would
> be too small and not blindly follow the request...I'll do better next
> time.

Yeah, I tend to push back on black and white process requirements
like this.  Everything has shades of grey. The process of responding
to so many small, trivial changes broken up like this is not an
efficient or effective use of developer time.

One function per patch makes sense when there are lots of changes to
a single function. But for stuff where there is only one or two
users (e.g. platform_discard, platform_findrawpath, platform*_block*
etc) splitting them in to single patches is not efficient.

I always think of it like this: would I like to have to reply to 29
emails just to add RVBs for such a change? Responding to 29 emails
is a decent chunk of time that every reviewer must invest. Is the
change complex enough to need such fine grained review effort.

In this case, I think no, especially because it hid where a
significant issue was introduced....

> > Also....
> > 
> > >   libxfs/init.c               | 32 ++++++------
> > >   libxfs/libxfs_io.h          |  2 +-
> > >   libxfs/libxfs_priv.h        |  3 +-
> > >   libxfs/rdwr.c               |  4 +-
> > >   libxfs/xfs_ag.c             |  6 +--
> > >   libxfs/xfs_attr_leaf.c      |  2 +-
> > >   libxfs/xfs_attr_remote.c    |  2 +-
> > >   libxfs/xfs_btree.c          |  4 +-
> > >   libxfs/xfs_da_btree.c       |  2 +-
> > >   libxfs/xfs_dir2_block.c     |  2 +-
> > >   libxfs/xfs_dir2_data.c      |  2 +-
> > >   libxfs/xfs_dir2_leaf.c      |  2 +-
> > >   libxfs/xfs_dir2_node.c      |  2 +-
> > >   libxfs/xfs_dquot_buf.c      |  2 +-
> > >   libxfs/xfs_ialloc.c         |  4 +-
> > >   libxfs/xfs_inode_buf.c      |  2 +-
> > >   libxfs/xfs_sb.c             |  6 +--
> > >   libxfs/xfs_symlink_remote.c |  2 +-
> > Why are all these libxfs files being changed?
> 
> I believe this is because of patch #6 - xfsprogs: Stop using
> platform_uuid_copy()
> 
> Here I dropped the usage of platform_uuid_copy() even in libxfs by:
> 
> 1) removing the uuid_copy() macro that was implemented by calling
>    platform_uuid_copy() in libxfs/libxfs_priv.h
> 
>  -#define uuid_copy(s,d) platform_uuid_copy((s),(d))
> 
> 2) using directly standard uuid_copy() instead
>    Which resulted into plenty of trivial changes all over libxfs, the
>    core of the changes being that uuid params are passed-by-value to
>    standard uuid_copy(), but to libxfs' uuid_copy() it is
>    passed-by-reference.
> 
>    E.g.
> - uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
> +               uuid_copy(agf->agf_uuid, mp->m_sb.sb_meta_uuid);

Ok, this is something that should definitely be mentioned in the
commit message because it's not just a rename.

As it is, this is the sort of problem we have to solve in userspace.
That was the purpose of the #define uuid_copy() you removed - it
translates the kernel API to the userspace API. We still need
something like that, which means the platform_uuid_copy()
change-over likely needs it's own patch and documentation (because
it's not just a straight rename anymore.

We do not want to make libxfs code unnecessarily different between
user and kernel space because that increases the overhead of porting
changes between kernel <-> userspace code bases. Hence we really
need to solve the "same function name, different calling convention"
in some way in userspace without requiring code diffs between kernel
and userspace.

Maybe some pre-processor magic can be done here in userspace. Maybe
we need to rename uuid_copy() in both the kernel and userspace, so
both have a static inline wrapper that can call the native
uuid_copy() with the correct parameters (another reason for it being
a separate patch so it can be easily ported).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com