On Tue, May 24, 2022 at 11:02:49AM +1000, Dave Chinner wrote:
> On Mon, May 23, 2022 at 05:35:40PM -0700, Darrick J. Wong wrote:
> > On Tue, May 24, 2022 at 08:56:43AM +1000, Dave Chinner wrote:
> > > On Mon, May 23, 2022 at 12:12:21PM -0700, Darrick J. Wong wrote:
> > > > On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> > > > > On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > > >
> > > > > > libxfs itself should never be messing with whether or not to enable
> > > > > > logging for extended attribute updates -- this decision should be made
> > > > > > on a case-by-case basis by libxfs callers. Move the code that actually
> > > > > > enables the log features to xfs_xattr.c, and adjust the callers.
> > > > > >
> > > > > > This removes an awkward coupling point between libxfs and what would be
> > > > > > libxlog, if the XFS log were actually its own library. Furthermore, it
> > > > > > makes bulk attribute updates and inode security initialization a tiny
> > > > > > bit more efficient, since they now avoid cycling the log feature between
> > > > > > every single xattr.
> > > > > >
> > > > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > > > ---
> > > > > > fs/xfs/libxfs/xfs_attr.c | 12 +-------
> > > > > > fs/xfs/xfs_acl.c | 10 +++++++
> > > > > > fs/xfs/xfs_ioctl.c | 22 +++++++++++++---
> > > > > > fs/xfs/xfs_ioctl.h | 2 +
> > > > > > fs/xfs/xfs_ioctl32.c | 4 ++-
> > > > > > fs/xfs/xfs_iops.c | 25 ++++++++++++++----
> > > > > > fs/xfs/xfs_log.c | 45 --------------------------------
> > > > > > fs/xfs/xfs_log.h | 1 -
> > > > > > fs/xfs/xfs_super.h | 2 +
> > > > > > fs/xfs/xfs_xattr.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 10 files changed, 120 insertions(+), 68 deletions(-)
> > > > >
> > > > > This seems like the wrong way to approach this. I would have defined
> > > > > a wrapper function for xfs_attr_set() to do the log state futzing,
> > > > > not moved it all into callers that don't need (or want) to know
> > > > > anything about how attrs are logged internally....
> > > >
> > > > I started doing this, and within a few hours realized that I'd set upon
> > > > yet *another* refactoring of xfs_attr_set. I'm not willing to do that
> > > > so soon after Allison's refactoring, so I'm dropping this patch.
> > >
> > > I don't see why this ends up being a problem - xfs_attr_set() is
> > > only called by code in fs/xfs/*.c, so adding a wrapper function
> > > that just does this:
> > >
> > > int
> > > xfs_attr_change(
> > > struct xfs_da_args *args)
> > > {
> > > struct xfs_mount *mp = args->dp->i_mount;
> > >
> > > if (xfs_has_larp(mp)) {
> > > error = xfs_attr_use_log_assist(mp);
> > > if (error)
> > > return error;
> > > }
> > >
> > > error = xfs_attr_set(args);
> > > if (xfs_has_larp(mp))
> >
> > Race condition here ^^^ if we race with someone changing the debug knob,
> > we'll either drop something we never took, or leak something we did
> > take.
>
> True, but largely irrelevant for the purposes of demonstration....
>
> >
> > > xlog_drop_incompat_feat(mp->m_log);
> > > return error;
> > > }
> > >
> > > into one of the files in fs/xfs will get this out of libxfs, won't
> > > it?
> > >
> > > What am I missing here?
> >
> > After the last year and a half I've gotten in the bad habit of trying to
> > anticipate the likely style objections of various reviewers to try to
> > get patches into upstream with as few objections as possible, which then
> > leads me down the path of more and more scope creep from the voices
> > inside my head:
> >
> > "These cleanups should be split into smaller changes for easy
> > backporting."
> >
> > "Setting xattr arguments via the da_args struct is a mess, make them
> > function parameters."
> >
> > "It's nasty to have xfs_attr_change take 7 parameters, just make an
> > xfs_attrchange_args struct with the pieces we need, and use it to fill
> > out the da args internally."
> >
> > "These calling conventions are still crap, transaction allocation
> > shouldn't even be in libxfs at all!"
> >
> > "Why don't you make attr_change have its own flags namespace, and then
> > set attr_filter and attr_flags from that? This would decouple the
> > interfaces and make them easier to figure out next time."
> >
> > "There are too many little xfs_attr functions and it's really hard to
> > grok what they all do."
>
> Yes, I understand that we need to re-layer the attr code to get rid
> of all the twisty passages that are all alike, but we don't need to
> do that right now and it avoids tying a simple change up in knots
> that prevent progress from being made.
>
> [FWIW, I have a basic plan for that - split all the sf/leaf/node
> stuff out of xfs_attr_leaf.c into xfs_attr_sf/leaf/node.c, move all
> the sf/leaf/node add/remove/change/lookup stuff out of xfs_attr.c
> into the above files. High level API is in xfs_attr.c (same as
> xfs_dir.c for directories) and everything external interfaces with
> that API. Which we can then tailor to just the information needed
> to set up attr and da_args inside xfs_attr.c....
>
> Then we can start cleaning up all the internal attr APIs, fix all
> the whacky quirks like sf add will do a remove if it's actually a
> replace, clean up the lookup interfaces, etc. ]
>
> > OTOH if you'd be willing to take just that attr_change bit above (with
> > the race condition fixed, I *would* send you that in patch form.
>
> Yes, I think that's just fine. Simple is often best...
Ok. Since I only just noticed the for-next update, I'll rebase on that,
make this change, and send that out for testing overnight.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
