On Tue, Feb 25, 2020 at 08:29:06PM -0700, Allison Collins wrote:
>
>
> On 2/25/20 6:27 AM, Brian Foster wrote:
> > 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..
> Oh, I see what you mean. No, I had done a review of the uses of
> xfs_attr_rmtval_remove and realized that it appears in both the set and
> remove paths. We use it in the set path when we're doing a rename, and need
> to remove the old attr.
>
> So in patch 13, we replace xfs_attr_rmtval_remove with xfs_attr_rmtval_unmap
> for the remove routines. But it's still in the set routines. So we cant
> take it away yet. Once we get through patch 14, it is no longer used and we
> can remove it. Did that make sense?
>
I don't think you need to take it away in this patch. What I'm
suggesting is breaking existing code into something like this:
int
__xfs_attr_rmtval_remove()
{
...
error = xfs_bunmapi(...);
...
if (!done)
return -EAGAIN;
...
}
int
xfs_attr_rmtval_remove()
{
error = xfs_attr_rmtval_invalidate();
do {
error = __xfs_attr_rmtval_remove();
...
xfs_defer_finish();
...
xfs_trans_roll_inode(...);
} while (-EAGAIN);
...
}
So we can continue to use both as needed without duplication and remove
the bits that become unused as the series progresses. Eventually we
could rename __xfs_attr_rmtval_remove() back to xfs_attr_rmtval_remove()
to maintain that it's essentially the same function and only the
transaction rolling bits are being factored out and eventually removed.
Brian
> >
> > > 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.
>
> Alrighty, I was thinking that if I reordered things such that modularizing
> appeared at the end of the set, that it would help people to see it in
> context with the states. But it sounds like people like it the other way
> around, it's easy enough to put back. Though I think we may still be stuck
> with 12 and 19 being apart unless 13 and 14 come together. :-(
>
> Allison
>
> >
> > 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
> > > > >
> > > >
> > >
> >
>
