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.
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. :-)
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
