On Sat, Aug 07, 2021 at 05:13:33PM +0200, Pavel Reichl wrote:

On 8/7/21 1:05 AM, Dave Chinner wrote:

quoted

On Fri, Aug 06, 2021 at 11:22:49PM +0200, Pavel Reichl wrote:

quoted

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

quoted

which we don't tend to do very often.

quoted

  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.

quoted

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:

quoted

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

quoted

Also....

quoted

  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