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.
> 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."
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.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
