| Message ID | 20210806212318.440144-1-preichl@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfsprogs: Drop the 'platform_' prefix | expand |
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, 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. 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. 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? $ git grep -l platform libxfs/ libxfs/init.c libxfs/libxfs_io.h libxfs/libxfs_priv.h libxfs/rdwr.c libxfs/xfs_log_format.h $ IOWs, there are calls to platform_*() functions in most of these libxfs files, so what is being changed here? If these are code changes, then they will end up needed to be ported across to the kernel libxfs, too. I did a quick scan of a few of the patches but I didn't land on the one that had these changes in it; another reason for not doing stuff in such fine grained ways. Hence it's not clear to me why renaming platform_*() functions would touch these files. Cheers, Dave.
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 :-). > 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. > > 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); > Cheers, > > Dave. Hi Dave, Thank you for you comments, please see reactions above. Have a nice day.
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.