On Mon, Feb 24, 2020 at 02:44:14PM -0700, Allison Collins wrote:
>
>
> On 2/24/20 6:40 AM, Brian Foster wrote:
> > On Sat, Feb 22, 2020 at 07:06:04PM -0700, Allison Collins wrote:
> > > This function is similar to xfs_attr_rmtval_remove, but adapted to return EAGAIN for
> > > new transactions. We will use this later when we introduce delayed attributes
> > >
> > > Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_attr_remote.c | 28 ++++++++++++++++++++++++++++
> > > fs/xfs/libxfs/xfs_attr_remote.h | 1 +
> > > 2 files changed, 29 insertions(+)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > index 3de2eec..da40f85 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > @@ -711,3 +711,31 @@ xfs_attr_rmtval_remove(
> > > }
> > > return 0;
> > > }
> > > +
> > > +/*
> > > + * Remove the value associated with an attribute by deleting the out-of-line
> > > + * buffer that it is stored on. Returns EAGAIN for the caller to refresh the
> > > + * transaction and recall the function
> > > + */
> > > +int
> > > +xfs_attr_rmtval_unmap(
> > > + struct xfs_da_args *args)
> > > +{
> > > + int error, done;
> > > +
> > > + /*
> > > + * Unmap value blocks for this attr. This is similar to
> > > + * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
> > > + * for new transactions
> > > + */
> > > + error = xfs_bunmapi(args->trans, args->dp,
> > > + args->rmtblkno, args->rmtblkcnt,
> > > + XFS_BMAPI_ATTRFORK, 1, &done);
> > > + if (error)
> > > + return error;
> > > +
> > > + if (!done)
> > > + return -EAGAIN;
> > > +
> > > + return 0;
> > > +}
> >
> > Hmm.. any reason this isn't a refactor of the existing remove function?
> > Just skipping to the end of the series, I see we leave the reference to
> > xfs_attr_rmtval_remove() (which no longer exists and so is not very
> > useful) in this comment as well as a stale function declaration in
> > xfs_attr_remote.h.
> >
> > I haven't grokked how this is used yet, but it seems like it would be
> > more appropriate to lift out the transaction handling from the original
> > function as we have throughout the rest of the code. That could also
> > mean creating a temporary wrapper (i.e., rmtval_remove() calls
> > rmtval_unmap()) for the loop/transaction code that could be removed
> > later if it ends up unused. Either way is much easier to follow than
> > creating a (currently unused) replacement..
> Yes, this came up in one of the other reviews. I thought about it, but then
> decided against it. xfs_attr_rmtval_remove disappears across patches 13 and
> 14. The use of xfs_attr_rmtval_remove is replaced with
> xfs_attr_rmtval_unmap when we finally yank out all the transaction code.
> The reason I dont want to do it all at once is because that would mean
> patches 12, 13, 14 and 19 would lump together to make the swap instantaneous
> in once patch.
>
Hmm.. I don't think we're talking about the same thing. If
xfs_attr_rmtval_remove() was broken down into two functions such that
one of the two looks exactly like the _unmap() variant, can't we just
remove the other half when it becomes unused and allow the _remove()
variant to exist with the implementation of _unmap() proposed here? This
seems fairly mechanical to me..
> I've been getting feedback that the set is really complicated, so I've been
> trying to find a way to organize it to help make it easier to review. So I
> thought isolating 13 and 14 to just the state machine would help. Thus I
> decided to keep patch 12 separate to take as much craziness out of 13 and 14
> as possible. Patches 12 and 19 seem like otherwise easy things for people
> to look at. Let me know your thoughts on this. :-)
>
I think doing as much refactoring of existing code as early as possible
will go a long way towards simplifying the complexity around the
introduction of the state bits.
Brian
> You are right about the stale comment though, I missed it while going back
> over the commentary at the top. Will fix.
>
> Allison
>
> >
> > Brian
> >
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > > index eff5f95..e06299a 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.h
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> > > @@ -14,4 +14,5 @@ int xfs_attr_rmtval_remove(struct xfs_da_args *args);
> > > int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map,
> > > xfs_buf_flags_t incore_flags);
> > > int xfs_attr_rmtval_invalidate(struct xfs_da_args *args);
> > > +int xfs_attr_rmtval_unmap(struct xfs_da_args *args);
> > > #endif /* __XFS_ATTR_REMOTE_H__ */
> > > --
> > > 2.7.4
> > >
> >
>
