* [PATCH v10 01/25] xfs: Add xfs_has_attr and subroutines
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:29 ` [PATCH v10 02/25] xfs: Check for -ENOATTR or -EEXIST Allison Collins
` (23 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
This patch adds a new functions to check for the existence of an
attribute. Subroutines are also added to handle the cases of leaf
blocks, nodes or shortform. Common code that appears in existing attr
add and remove functions have been factored out to help reduce the
appearance of duplicated code. We will need these routines later for
delayed attributes since delayed operations cannot return error codes.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 182 ++++++++++++++++++++++++++++--------------
fs/xfs/libxfs/xfs_attr.h | 1 +
fs/xfs/libxfs/xfs_attr_leaf.c | 97 +++++++++++++++-------
fs/xfs/libxfs/xfs_attr_leaf.h | 3 +
4 files changed, 191 insertions(+), 92 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 3b1bd6e..cecc794 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -46,6 +46,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
/*
* Internal routines when attribute list is more than one block.
@@ -53,6 +54,8 @@ STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
STATIC int xfs_attr_node_get(xfs_da_args_t *args);
STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
+ struct xfs_da_state **state);
STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
@@ -261,6 +264,37 @@ xfs_attr_set_args(
}
/*
+ * Return EEXIST if attr is found, or ENOATTR if not
+ */
+int
+xfs_has_attr(
+ struct xfs_da_args *args)
+{
+ struct xfs_inode *dp = args->dp;
+ struct xfs_buf *bp = NULL;
+ int error;
+
+ if (!xfs_inode_hasattr(dp))
+ return -ENOATTR;
+
+ if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
+ ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
+ return xfs_attr_sf_findname(args, NULL, NULL);
+ }
+
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+ error = xfs_attr_leaf_hasname(args, &bp);
+
+ if (bp)
+ xfs_trans_brelse(args->trans, bp);
+
+ return error;
+ }
+
+ return xfs_attr_node_hasname(args, NULL);
+}
+
+/*
* Remove the attribute specified in @args.
*/
int
@@ -469,26 +503,19 @@ STATIC int
xfs_attr_leaf_addname(
struct xfs_da_args *args)
{
- struct xfs_inode *dp;
struct xfs_buf *bp;
int retval, error, forkoff;
+ struct xfs_inode *dp = args->dp;
trace_xfs_attr_leaf_addname(args);
/*
- * Read the (only) block in the attribute list in.
- */
- dp = args->dp;
- args->blkno = 0;
- error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, &bp);
- if (error)
- return error;
-
- /*
* Look up the given attribute in the leaf block. Figure out if
* the given flags produce an error or call for an atomic rename.
*/
- retval = xfs_attr3_leaf_lookup_int(bp, args);
+ retval = xfs_attr_leaf_hasname(args, &bp);
+ if (retval != -ENOATTR && retval != -EEXIST)
+ return retval;
if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
goto out_brelse;
if (retval == -EEXIST) {
@@ -640,6 +667,27 @@ xfs_attr_leaf_addname(
}
/*
+ * Return EEXIST if attr is found, or ENOATTR if not
+ */
+STATIC int
+xfs_attr_leaf_hasname(
+ struct xfs_da_args *args,
+ struct xfs_buf **bp)
+{
+ int error = 0;
+
+ error = xfs_attr3_leaf_read(args->trans, args->dp, 0, bp);
+ if (error)
+ return error;
+
+ error = xfs_attr3_leaf_lookup_int(*bp, args);
+ if (error != -ENOATTR && error != -EEXIST)
+ xfs_trans_brelse(args->trans, *bp);
+
+ return error;
+}
+
+/*
* Remove a name from the leaf attribute list structure
*
* This leaf block cannot have a "remote" value, we only call this routine
@@ -659,16 +707,14 @@ xfs_attr_leaf_removename(
* Remove the attribute.
*/
dp = args->dp;
- args->blkno = 0;
- error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, &bp);
- if (error)
- return error;
- error = xfs_attr3_leaf_lookup_int(bp, args);
+ error = xfs_attr_leaf_hasname(args, &bp);
+
if (error == -ENOATTR) {
xfs_trans_brelse(args->trans, bp);
return error;
- }
+ } else if (error != -EEXIST)
+ return error;
xfs_attr3_leaf_remove(bp, args);
@@ -703,21 +749,56 @@ xfs_attr_leaf_get(xfs_da_args_t *args)
trace_xfs_attr_leaf_get(args);
- args->blkno = 0;
- error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno, &bp);
- if (error)
- return error;
+ error = xfs_attr_leaf_hasname(args, &bp);
- error = xfs_attr3_leaf_lookup_int(bp, args);
- if (error != -EEXIST) {
+ if (error == -ENOATTR) {
xfs_trans_brelse(args->trans, bp);
return error;
- }
+ } else if (error != -EEXIST)
+ return error;
+
+
error = xfs_attr3_leaf_getvalue(bp, args);
xfs_trans_brelse(args->trans, bp);
return error;
}
+/*
+ * Return EEXIST if attr is found, or ENOATTR if not
+ * statep: If not null is set to point at the found state. Caller will
+ * be responsible for freeing the state in this case.
+ */
+STATIC int
+xfs_attr_node_hasname(
+ struct xfs_da_args *args,
+ struct xfs_da_state **statep)
+{
+ struct xfs_da_state *state;
+ int retval, error;
+
+ state = xfs_da_state_alloc();
+ state->args = args;
+ state->mp = args->dp->i_mount;
+
+ if (statep != NULL)
+ *statep = NULL;
+
+ /*
+ * Search to see if name exists, and get back a pointer to it.
+ */
+ error = xfs_da3_node_lookup_int(state, &retval);
+ if (error) {
+ xfs_da_state_free(state);
+ return error;
+ }
+
+ if (statep != NULL)
+ *statep = state;
+ else
+ xfs_da_state_free(state);
+ return retval;
+}
+
/*========================================================================
* External routines when attribute list size > geo->blksize
*========================================================================*/
@@ -750,17 +831,14 @@ xfs_attr_node_addname(
dp = args->dp;
mp = dp->i_mount;
restart:
- state = xfs_da_state_alloc();
- state->args = args;
- state->mp = mp;
-
/*
* Search to see if name already exists, and get back a pointer
* to where it should go.
*/
- error = xfs_da3_node_lookup_int(state, &retval);
- if (error)
+ retval = xfs_attr_node_hasname(args, &state);
+ if (retval != -ENOATTR && retval != -EEXIST)
goto out;
+
blk = &state->path.blk[ state->path.active-1 ];
ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
@@ -965,29 +1043,15 @@ xfs_attr_node_removename(
{
struct xfs_da_state *state;
struct xfs_da_state_blk *blk;
- struct xfs_inode *dp;
struct xfs_buf *bp;
int retval, error, forkoff;
+ struct xfs_inode *dp = args->dp;
trace_xfs_attr_node_removename(args);
- /*
- * Tie a string around our finger to remind us where we are.
- */
- dp = args->dp;
- state = xfs_da_state_alloc();
- state->args = args;
- state->mp = dp->i_mount;
-
- /*
- * Search to see if name exists, and get back a pointer to it.
- */
- error = xfs_da3_node_lookup_int(state, &retval);
- if (error || (retval != -EEXIST)) {
- if (error == 0)
- error = retval;
+ error = xfs_attr_node_hasname(args, &state);
+ if (error != -EEXIST)
goto out;
- }
/*
* If there is an out-of-line value, de-allocate the blocks.
@@ -1082,7 +1146,8 @@ xfs_attr_node_removename(
error = 0;
out:
- xfs_da_state_free(state);
+ if (state)
+ xfs_da_state_free(state);
return error;
}
@@ -1202,31 +1267,23 @@ xfs_attr_node_get(xfs_da_args_t *args)
{
xfs_da_state_t *state;
xfs_da_state_blk_t *blk;
- int error, retval;
+ int error;
int i;
trace_xfs_attr_node_get(args);
- state = xfs_da_state_alloc();
- state->args = args;
- state->mp = args->dp->i_mount;
-
/*
* Search to see if name exists, and get back a pointer to it.
*/
- error = xfs_da3_node_lookup_int(state, &retval);
- if (error) {
- retval = error;
- goto out_release;
- }
- if (retval != -EEXIST)
+ error = xfs_attr_node_hasname(args, &state);
+ if (error != -EEXIST)
goto out_release;
/*
* Get the value, local or "remote"
*/
blk = &state->path.blk[state->path.active - 1];
- retval = xfs_attr3_leaf_getvalue(blk->bp, args);
+ error = xfs_attr3_leaf_getvalue(blk->bp, args);
/*
* If not in a transaction, we have to release all the buffers.
@@ -1237,8 +1294,9 @@ xfs_attr_node_get(xfs_da_args_t *args)
state->path.blk[i].bp = NULL;
}
- xfs_da_state_free(state);
- return retval;
+ if (state)
+ xfs_da_state_free(state);
+ return error;
}
/* Returns true if the attribute entry name is valid. */
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index db47176..3e97a93 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -89,6 +89,7 @@ int xfs_attr_get_ilocked(struct xfs_da_args *args);
int xfs_attr_get(struct xfs_da_args *args);
int xfs_attr_set(struct xfs_da_args *args);
int xfs_attr_set_args(struct xfs_da_args *args);
+int xfs_has_attr(struct xfs_da_args *args);
int xfs_attr_remove_args(struct xfs_da_args *args);
bool xfs_attr_namecheck(const void *name, size_t length);
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 2f7e89e..e40b08c 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -660,18 +660,63 @@ xfs_attr_shortform_create(
}
/*
+ * Return -EEXIST if attr is found, or -ENOATTR if not
+ * args: args containing attribute name and namelen
+ * sfep: If not null, pointer will be set to the last attr entry found on
+ -EEXIST. On -ENOATTR pointer is left at the last entry in the list
+ * basep: If not null, pointer is set to the byte offset of the entry in the
+ * list on -EEXIST. On -ENOATTR, pointer is left at the byte offset of
+ * the last entry in the list
+ */
+int
+xfs_attr_sf_findname(
+ struct xfs_da_args *args,
+ struct xfs_attr_sf_entry **sfep,
+ unsigned int *basep)
+{
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ unsigned int base = sizeof(struct xfs_attr_sf_hdr);
+ int size = 0;
+ int end;
+ int i;
+
+ sf = (struct xfs_attr_shortform *)args->dp->i_afp->if_u1.if_data;
+ sfe = &sf->list[0];
+ end = sf->hdr.count;
+ for (i = 0; i < end; sfe = XFS_ATTR_SF_NEXTENTRY(sfe),
+ base += size, i++) {
+ size = XFS_ATTR_SF_ENTSIZE(sfe);
+ if (!xfs_attr_match(args, sfe->namelen, sfe->nameval,
+ sfe->flags))
+ continue;
+ break;
+ }
+
+ if (sfep != NULL)
+ *sfep = sfe;
+
+ if (basep != NULL)
+ *basep = base;
+
+ if (i == end)
+ return -ENOATTR;
+ return -EEXIST;
+}
+
+/*
* Add a name/value pair to the shortform attribute list.
* Overflow from the inode has already been checked for.
*/
void
-xfs_attr_shortform_add(xfs_da_args_t *args, int forkoff)
+xfs_attr_shortform_add(struct xfs_da_args *args, int forkoff)
{
- xfs_attr_shortform_t *sf;
- xfs_attr_sf_entry_t *sfe;
- int i, offset, size;
- xfs_mount_t *mp;
- xfs_inode_t *dp;
- struct xfs_ifork *ifp;
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ int offset, size, error;
+ struct xfs_mount *mp;
+ struct xfs_inode *dp;
+ struct xfs_ifork *ifp;
trace_xfs_attr_sf_add(args);
@@ -682,11 +727,8 @@ xfs_attr_shortform_add(xfs_da_args_t *args, int forkoff)
ifp = dp->i_afp;
ASSERT(ifp->if_flags & XFS_IFINLINE);
sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
- sfe = &sf->list[0];
- for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
- ASSERT(!xfs_attr_match(args, sfe->namelen, sfe->nameval,
- sfe->flags));
- }
+ error = xfs_attr_sf_findname(args, &sfe, NULL);
+ ASSERT(error != -EEXIST);
offset = (char *)sfe - (char *)sf;
size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
@@ -728,31 +770,26 @@ xfs_attr_fork_remove(
* Remove an attribute from the shortform attribute list structure.
*/
int
-xfs_attr_shortform_remove(xfs_da_args_t *args)
+xfs_attr_shortform_remove(struct xfs_da_args *args)
{
- xfs_attr_shortform_t *sf;
- xfs_attr_sf_entry_t *sfe;
- int base, size=0, end, totsize, i;
- xfs_mount_t *mp;
- xfs_inode_t *dp;
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ int size = 0, end, totsize;
+ unsigned int base;
+ struct xfs_mount *mp;
+ struct xfs_inode *dp;
+ int error;
trace_xfs_attr_sf_remove(args);
dp = args->dp;
mp = dp->i_mount;
- base = sizeof(xfs_attr_sf_hdr_t);
sf = (xfs_attr_shortform_t *)dp->i_afp->if_u1.if_data;
- sfe = &sf->list[0];
- end = sf->hdr.count;
- for (i = 0; i < end; sfe = XFS_ATTR_SF_NEXTENTRY(sfe),
- base += size, i++) {
- size = XFS_ATTR_SF_ENTSIZE(sfe);
- if (xfs_attr_match(args, sfe->namelen, sfe->nameval,
- sfe->flags))
- break;
- }
- if (i == end)
- return -ENOATTR;
+
+ error = xfs_attr_sf_findname(args, &sfe, &base);
+ if (error != -EEXIST)
+ return error;
+ size = XFS_ATTR_SF_ENTSIZE(sfe);
/*
* Fix up the attribute fork data, covering the hole
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index 5be6be3..9b1c59f 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -52,6 +52,9 @@ int xfs_attr_shortform_getvalue(struct xfs_da_args *args);
int xfs_attr_shortform_to_leaf(struct xfs_da_args *args,
struct xfs_buf **leaf_bp);
int xfs_attr_shortform_remove(struct xfs_da_args *args);
+int xfs_attr_sf_findname(struct xfs_da_args *args,
+ struct xfs_attr_sf_entry **sfep,
+ unsigned int *basep);
int xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode *dp);
int xfs_attr_shortform_bytesfit(struct xfs_inode *dp, int bytes);
xfs_failaddr_t xfs_attr_shortform_verify(struct xfs_inode *ip);
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 02/25] xfs: Check for -ENOATTR or -EEXIST
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
2020-06-25 23:29 ` [PATCH v10 01/25] xfs: Add xfs_has_attr and subroutines Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:29 ` [PATCH v10 03/25] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
` (22 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
Delayed operations cannot return error codes. So we must check for
these conditions first before starting set or remove operations
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index cecc794..ff28d6e 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -404,6 +404,15 @@ xfs_attr_set(
args->total, 0, quota_flags);
if (error)
goto out_trans_cancel;
+
+ error = xfs_has_attr(args);
+ if (error == -EEXIST && (args->attr_flags & XATTR_CREATE))
+ goto out_trans_cancel;
+ if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
+ goto out_trans_cancel;
+ if (error != -ENOATTR && error != -EEXIST)
+ goto out_trans_cancel;
+
error = xfs_attr_set_args(args);
if (error)
goto out_trans_cancel;
@@ -411,6 +420,10 @@ xfs_attr_set(
if (!args->trans)
goto out_unlock;
} else {
+ error = xfs_has_attr(args);
+ if (error != -EEXIST)
+ goto out_trans_cancel;
+
error = xfs_attr_remove_args(args);
if (error)
goto out_trans_cancel;
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 03/25] xfs: Factor out new helper functions xfs_attr_rmtval_set
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
2020-06-25 23:29 ` [PATCH v10 01/25] xfs: Add xfs_has_attr and subroutines Allison Collins
2020-06-25 23:29 ` [PATCH v10 02/25] xfs: Check for -ENOATTR or -EEXIST Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:29 ` [PATCH v10 04/25] xfs: Pull up trans handling in xfs_attr3_leaf_flipflags Allison Collins
` (21 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
Break xfs_attr_rmtval_set into two helper functions
xfs_attr_rmt_find_hole and xfs_attr_rmtval_set_value.
xfs_attr_rmtval_set rolls the transaction between the helpers, but
delayed operations cannot. We will use the helpers later when
constructing new delayed attribute routines.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr_remote.c | 149 +++++++++++++++++++++++++---------------
1 file changed, 92 insertions(+), 57 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 01ad7f3..f825eed 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -440,32 +440,23 @@ xfs_attr_rmtval_get(
}
/*
- * Write the value associated with an attribute into the out-of-line buffer
- * that we have defined for it.
+ * Find a "hole" in the attribute address space large enough for us to drop the
+ * new attribute's value into
*/
-int
-xfs_attr_rmtval_set(
+STATIC int
+xfs_attr_rmt_find_hole(
struct xfs_da_args *args)
{
struct xfs_inode *dp = args->dp;
struct xfs_mount *mp = dp->i_mount;
- struct xfs_bmbt_irec map;
- xfs_dablk_t lblkno;
- xfs_fileoff_t lfileoff = 0;
- uint8_t *src = args->value;
- int blkcnt;
- int valuelen;
- int nmap;
int error;
- int offset = 0;
-
- trace_xfs_attr_rmtval_set(args);
+ int blkcnt;
+ xfs_fileoff_t lfileoff = 0;
/*
- * Find a "hole" in the attribute address space large enough for
- * us to drop the new attribute's value into. Because CRC enable
- * attributes have headers, we can't just do a straight byte to FSB
- * conversion and have to take the header space into account.
+ * Because CRC enable attributes have headers, we can't just do a
+ * straight byte to FSB conversion and have to take the header space
+ * into account.
*/
blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
@@ -473,48 +464,26 @@ xfs_attr_rmtval_set(
if (error)
return error;
- args->rmtblkno = lblkno = (xfs_dablk_t)lfileoff;
+ args->rmtblkno = (xfs_dablk_t)lfileoff;
args->rmtblkcnt = blkcnt;
- /*
- * Roll through the "value", allocating blocks on disk as required.
- */
- while (blkcnt > 0) {
- /*
- * Allocate a single extent, up to the size of the value.
- *
- * Note that we have to consider this a data allocation as we
- * write the remote attribute without logging the contents.
- * Hence we must ensure that we aren't using blocks that are on
- * the busy list so that we don't overwrite blocks which have
- * recently been freed but their transactions are not yet
- * committed to disk. If we overwrite the contents of a busy
- * extent and then crash then the block may not contain the
- * correct metadata after log recovery occurs.
- */
- nmap = 1;
- error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
- blkcnt, XFS_BMAPI_ATTRFORK, args->total, &map,
- &nmap);
- if (error)
- return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
-
- ASSERT(nmap == 1);
- ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
- (map.br_startblock != HOLESTARTBLOCK));
- lblkno += map.br_blockcount;
- blkcnt -= map.br_blockcount;
+ return 0;
+}
- /*
- * Start the next trans in the chain.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- return error;
- }
+STATIC int
+xfs_attr_rmtval_set_value(
+ struct xfs_da_args *args)
+{
+ struct xfs_inode *dp = args->dp;
+ struct xfs_mount *mp = dp->i_mount;
+ struct xfs_bmbt_irec map;
+ xfs_dablk_t lblkno;
+ uint8_t *src = args->value;
+ int blkcnt;
+ int valuelen;
+ int nmap;
+ int error;
+ int offset = 0;
/*
* Roll through the "value", copying the attribute value to the
@@ -595,6 +564,72 @@ xfs_attr_rmtval_stale(
}
/*
+ * Write the value associated with an attribute into the out-of-line buffer
+ * that we have defined for it.
+ */
+int
+xfs_attr_rmtval_set(
+ struct xfs_da_args *args)
+{
+ struct xfs_inode *dp = args->dp;
+ struct xfs_bmbt_irec map;
+ xfs_dablk_t lblkno;
+ int blkcnt;
+ int nmap;
+ int error;
+
+ trace_xfs_attr_rmtval_set(args);
+
+ error = xfs_attr_rmt_find_hole(args);
+ if (error)
+ return error;
+
+ blkcnt = args->rmtblkcnt;
+ lblkno = (xfs_dablk_t)args->rmtblkno;
+ /*
+ * Roll through the "value", allocating blocks on disk as required.
+ */
+ while (blkcnt > 0) {
+ /*
+ * Allocate a single extent, up to the size of the value.
+ *
+ * Note that we have to consider this a data allocation as we
+ * write the remote attribute without logging the contents.
+ * Hence we must ensure that we aren't using blocks that are on
+ * the busy list so that we don't overwrite blocks which have
+ * recently been freed but their transactions are not yet
+ * committed to disk. If we overwrite the contents of a busy
+ * extent and then crash then the block may not contain the
+ * correct metadata after log recovery occurs.
+ */
+ nmap = 1;
+ error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
+ blkcnt, XFS_BMAPI_ATTRFORK, args->total, &map,
+ &nmap);
+ if (error)
+ return error;
+ error = xfs_defer_finish(&args->trans);
+ if (error)
+ return error;
+
+ ASSERT(nmap == 1);
+ ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
+ (map.br_startblock != HOLESTARTBLOCK));
+ lblkno += map.br_blockcount;
+ blkcnt -= map.br_blockcount;
+
+ /*
+ * Start the next trans in the chain.
+ */
+ error = xfs_trans_roll_inode(&args->trans, dp);
+ if (error)
+ return error;
+ }
+
+ return xfs_attr_rmtval_set_value(args);
+}
+
+/*
* Remove the value associated with an attribute by deleting the
* out-of-line buffer that it is stored on.
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 04/25] xfs: Pull up trans handling in xfs_attr3_leaf_flipflags
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (2 preceding siblings ...)
2020-06-25 23:29 ` [PATCH v10 03/25] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:29 ` [PATCH v10 05/25] xfs: Split apart xfs_attr_leaf_addname Allison Collins
` (20 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
Since delayed operations cannot roll transactions, pull up the
transaction handling into the calling function
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr.c | 14 ++++++++++++++
fs/xfs/libxfs/xfs_attr_leaf.c | 7 +------
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index ff28d6e..28a9f8e 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -622,6 +622,13 @@ xfs_attr_leaf_addname(
error = xfs_attr3_leaf_flipflags(args);
if (error)
return error;
+ /*
+ * Commit the flag value change and start the next trans in
+ * series.
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ return error;
/*
* Dismantle the "old" attribute/value pair by removing
@@ -968,6 +975,13 @@ xfs_attr_node_addname(
error = xfs_attr3_leaf_flipflags(args);
if (error)
goto out;
+ /*
+ * Commit the flag value change and start the next trans in
+ * series
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ goto out;
/*
* Dismantle the "old" attribute/value pair by removing
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index e40b08c..97c0d72 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -2948,10 +2948,5 @@ xfs_attr3_leaf_flipflags(
XFS_DA_LOGRANGE(leaf2, name_rmt, sizeof(*name_rmt)));
}
- /*
- * Commit the flag value change and start the next trans in series.
- */
- error = xfs_trans_roll_inode(&args->trans, args->dp);
-
- return error;
+ return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 05/25] xfs: Split apart xfs_attr_leaf_addname
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (3 preceding siblings ...)
2020-06-25 23:29 ` [PATCH v10 04/25] xfs: Pull up trans handling in xfs_attr3_leaf_flipflags Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:29 ` [PATCH v10 06/25] xfs: Refactor xfs_attr_try_sf_addname Allison Collins
` (19 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
Split out new helper function xfs_attr_leaf_try_add from
xfs_attr_leaf_addname. Because new delayed attribute routines cannot
roll transactions, we split off the parts of xfs_attr_leaf_addname that
we can use, and move the commit into the calling function.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr.c | 95 +++++++++++++++++++++++++++++++-----------------
1 file changed, 61 insertions(+), 34 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 28a9f8e..6fc6dc6 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -256,10 +256,31 @@ xfs_attr_set_args(
}
}
- if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
error = xfs_attr_leaf_addname(args);
- else
- error = xfs_attr_node_addname(args);
+ if (error != -ENOSPC)
+ return error;
+
+ /*
+ * Finish any deferred work items and roll the transaction once
+ * more. The goal here is to call node_addname with the inode
+ * and transaction in the same state (inode locked and joined,
+ * transaction clean) no matter how we got to this step.
+ */
+ error = xfs_defer_finish(&args->trans);
+ if (error)
+ return error;
+
+ /*
+ * Commit the current trans (including the inode) and
+ * start a new one.
+ */
+ error = xfs_trans_roll_inode(&args->trans, dp);
+ if (error)
+ return error;
+ }
+
+ error = xfs_attr_node_addname(args);
return error;
}
@@ -507,20 +528,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
*========================================================================*/
/*
- * Add a name to the leaf attribute list structure
+ * Tries to add an attribute to an inode in leaf form
*
- * This leaf block cannot have a "remote" value, we only call this routine
- * if bmap_one_block() says there is only one block (ie: no remote blks).
+ * This function is meant to execute as part of a delayed operation and leaves
+ * the transaction handling to the caller. On success the attribute is added
+ * and the inode and transaction are left dirty. If there is not enough space,
+ * the attr data is converted to node format and -ENOSPC is returned. Caller is
+ * responsible for handling the dirty inode and transaction or adding the attr
+ * in node format.
*/
STATIC int
-xfs_attr_leaf_addname(
- struct xfs_da_args *args)
+xfs_attr_leaf_try_add(
+ struct xfs_da_args *args,
+ struct xfs_buf *bp)
{
- struct xfs_buf *bp;
- int retval, error, forkoff;
- struct xfs_inode *dp = args->dp;
-
- trace_xfs_attr_leaf_addname(args);
+ int retval, error;
/*
* Look up the given attribute in the leaf block. Figure out if
@@ -562,31 +584,39 @@ xfs_attr_leaf_addname(
retval = xfs_attr3_leaf_add(bp, args);
if (retval == -ENOSPC) {
/*
- * Promote the attribute list to the Btree format, then
- * Commit that transaction so that the node_addname() call
- * can manage its own transactions.
+ * Promote the attribute list to the Btree format. Unless an
+ * error occurs, retain the -ENOSPC retval
*/
error = xfs_attr3_leaf_to_node(args);
if (error)
return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
+ }
+ return retval;
+out_brelse:
+ xfs_trans_brelse(args->trans, bp);
+ return retval;
+}
- /*
- * Commit the current trans (including the inode) and start
- * a new one.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- return error;
- /*
- * Fob the whole rest of the problem off on the Btree code.
- */
- error = xfs_attr_node_addname(args);
+/*
+ * Add a name to the leaf attribute list structure
+ *
+ * This leaf block cannot have a "remote" value, we only call this routine
+ * if bmap_one_block() says there is only one block (ie: no remote blks).
+ */
+STATIC int
+xfs_attr_leaf_addname(
+ struct xfs_da_args *args)
+{
+ int error, forkoff;
+ struct xfs_buf *bp = NULL;
+ struct xfs_inode *dp = args->dp;
+
+ trace_xfs_attr_leaf_addname(args);
+
+ error = xfs_attr_leaf_try_add(args, bp);
+ if (error)
return error;
- }
/*
* Commit the transaction that added the attr name so that
@@ -681,9 +711,6 @@ xfs_attr_leaf_addname(
error = xfs_attr3_leaf_clearflag(args);
}
return error;
-out_brelse:
- xfs_trans_brelse(args->trans, bp);
- return retval;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 06/25] xfs: Refactor xfs_attr_try_sf_addname
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (4 preceding siblings ...)
2020-06-25 23:29 ` [PATCH v10 05/25] xfs: Split apart xfs_attr_leaf_addname Allison Collins
@ 2020-06-25 23:29 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 07/25] xfs: Pull up trans roll from xfs_attr3_leaf_setflag Allison Collins
` (18 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:29 UTC (permalink / raw)
To: linux-xfs
To help pre-simplify xfs_attr_set_args, we need to hoist transaction
handling up, while modularizing the adjacent code down into helpers. In
this patch, hoist the commit in xfs_attr_try_sf_addname up into the
calling function, and also pull the attr list creation down.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 6fc6dc6..9e1d389 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -178,8 +178,13 @@ xfs_attr_try_sf_addname(
struct xfs_da_args *args)
{
- struct xfs_mount *mp = dp->i_mount;
- int error, error2;
+ int error;
+
+ /*
+ * Build initial attribute list (if required).
+ */
+ if (dp->i_afp->if_format == XFS_DINODE_FMT_EXTENTS)
+ xfs_attr_shortform_create(args);
error = xfs_attr_shortform_addname(args);
if (error == -ENOSPC)
@@ -192,12 +197,10 @@ xfs_attr_try_sf_addname(
if (!error && !(args->op_flags & XFS_DA_OP_NOTIME))
xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
- if (mp->m_flags & XFS_MOUNT_WSYNC)
+ if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC)
xfs_trans_set_sync(args->trans);
- error2 = xfs_trans_commit(args->trans);
- args->trans = NULL;
- return error ? error : error2;
+ return error;
}
/*
@@ -209,7 +212,7 @@ xfs_attr_set_args(
{
struct xfs_inode *dp = args->dp;
struct xfs_buf *leaf_bp = NULL;
- int error;
+ int error, error2 = 0;
/*
* If the attribute list is non-existent or a shortform list,
@@ -220,17 +223,14 @@ xfs_attr_set_args(
dp->i_afp->if_nextents == 0)) {
/*
- * Build initial attribute list (if required).
- */
- if (dp->i_afp->if_format == XFS_DINODE_FMT_EXTENTS)
- xfs_attr_shortform_create(args);
-
- /*
* Try to add the attr to the attribute list in the inode.
*/
error = xfs_attr_try_sf_addname(dp, args);
- if (error != -ENOSPC)
- return error;
+ if (error != -ENOSPC) {
+ error2 = xfs_trans_commit(args->trans);
+ args->trans = NULL;
+ return error ? error : error2;
+ }
/*
* It won't fit in the shortform, transform to a leaf block.
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 07/25] xfs: Pull up trans roll from xfs_attr3_leaf_setflag
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (5 preceding siblings ...)
2020-06-25 23:29 ` [PATCH v10 06/25] xfs: Refactor xfs_attr_try_sf_addname Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 08/25] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
` (17 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
New delayed allocation routines cannot be handling transactions so
pull them up into the calling functions
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr.c | 5 +++++
fs/xfs/libxfs/xfs_attr_leaf.c | 5 +----
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 9e1d389..8d735210 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1132,6 +1132,11 @@ xfs_attr_node_removename(
error = xfs_attr3_leaf_setflag(args);
if (error)
goto out;
+
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ goto out;
+
error = xfs_attr_rmtval_remove(args);
if (error)
goto out;
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 97c0d72..c500eba8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -2830,10 +2830,7 @@ xfs_attr3_leaf_setflag(
XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
}
- /*
- * Commit the flag value change and start the next trans in series.
- */
- return xfs_trans_roll_inode(&args->trans, args->dp);
+ return 0;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 08/25] xfs: Factor out xfs_attr_rmtval_invalidate
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (6 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 07/25] xfs: Pull up trans roll from xfs_attr3_leaf_setflag Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 09/25] xfs: Pull up trans roll in xfs_attr3_leaf_clearflag Allison Collins
` (16 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Because new delayed attribute routines cannot roll transactions, we
carve off the parts of xfs_attr_rmtval_remove that we can use. This
will help to reduce repetitive code later when we introduce delayed
attributes.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr_remote.c | 26 +++++++++++++++++++++-----
fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index f825eed..4d51969 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -634,15 +634,12 @@ xfs_attr_rmtval_set(
* out-of-line buffer that it is stored on.
*/
int
-xfs_attr_rmtval_remove(
+xfs_attr_rmtval_invalidate(
struct xfs_da_args *args)
{
xfs_dablk_t lblkno;
int blkcnt;
int error;
- int done;
-
- trace_xfs_attr_rmtval_remove(args);
/*
* Roll through the "value", invalidating the attribute value's blocks.
@@ -670,13 +667,32 @@ xfs_attr_rmtval_remove(
lblkno += map.br_blockcount;
blkcnt -= map.br_blockcount;
}
+ return 0;
+}
+/*
+ * Remove the value associated with an attribute by deleting the
+ * out-of-line buffer that it is stored on.
+ */
+int
+xfs_attr_rmtval_remove(
+ struct xfs_da_args *args)
+{
+ xfs_dablk_t lblkno;
+ int blkcnt;
+ int error = 0;
+ int done = 0;
+
+ trace_xfs_attr_rmtval_remove(args);
+
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
/*
* Keep de-allocating extents until the remote-value region is gone.
*/
lblkno = args->rmtblkno;
blkcnt = args->rmtblkcnt;
- done = 0;
while (!done) {
error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt,
XFS_BMAPI_ATTRFORK, 1, &done);
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index e1144f2..3616e88 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -13,5 +13,5 @@ int xfs_attr_rmtval_set(struct xfs_da_args *args);
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);
#endif /* __XFS_ATTR_REMOTE_H__ */
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 09/25] xfs: Pull up trans roll in xfs_attr3_leaf_clearflag
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (7 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 08/25] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 10/25] xfs: Refactor xfs_attr_rmtval_remove Allison Collins
` (15 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
New delayed allocation routines cannot be handling transactions so
pull them out into the calling functions
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
---
fs/xfs/libxfs/xfs_attr.c | 16 ++++++++++++++++
fs/xfs/libxfs/xfs_attr_leaf.c | 5 +----
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 8d735210..df550b4 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -709,6 +709,14 @@ xfs_attr_leaf_addname(
* Added a "remote" value, just clear the incomplete flag.
*/
error = xfs_attr3_leaf_clearflag(args);
+ if (error)
+ return error;
+
+ /*
+ * Commit the flag value change and start the next trans in
+ * series.
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
}
return error;
}
@@ -1073,6 +1081,14 @@ xfs_attr_node_addname(
error = xfs_attr3_leaf_clearflag(args);
if (error)
goto out;
+
+ /*
+ * Commit the flag value change and start the next trans in
+ * series.
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ goto out;
}
retval = error = 0;
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index c500eba8..351351c 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -2779,10 +2779,7 @@ xfs_attr3_leaf_clearflag(
XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt)));
}
- /*
- * Commit the flag value change and start the next trans in series.
- */
- return xfs_trans_roll_inode(&args->trans, args->dp);
+ return 0;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 10/25] xfs: Refactor xfs_attr_rmtval_remove
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (8 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 09/25] xfs: Pull up trans roll in xfs_attr3_leaf_clearflag Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 11/25] xfs: Pull up xfs_attr_rmtval_invalidate Allison Collins
` (14 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Refactor xfs_attr_rmtval_remove to add helper function
__xfs_attr_rmtval_remove. We will use this later when we introduce
delayed attributes. This function will eventually replace
xfs_attr_rmtval_remove
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr_remote.c | 46 ++++++++++++++++++++++++++++++++---------
fs/xfs/libxfs/xfs_attr_remote.h | 1 +
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 4d51969..9b4c173 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -681,7 +681,7 @@ xfs_attr_rmtval_remove(
xfs_dablk_t lblkno;
int blkcnt;
int error = 0;
- int done = 0;
+ int retval = 0;
trace_xfs_attr_rmtval_remove(args);
@@ -693,14 +693,10 @@ xfs_attr_rmtval_remove(
*/
lblkno = args->rmtblkno;
blkcnt = args->rmtblkcnt;
- while (!done) {
- error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt,
- XFS_BMAPI_ATTRFORK, 1, &done);
- if (error)
- return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
+ do {
+ retval = __xfs_attr_rmtval_remove(args);
+ if (retval && retval != EAGAIN)
+ return retval;
/*
* Close out trans and start the next one in the chain.
@@ -708,6 +704,36 @@ xfs_attr_rmtval_remove(
error = xfs_trans_roll_inode(&args->trans, args->dp);
if (error)
return error;
- }
+ } while (retval == -EAGAIN);
+
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 re-call the function
+ */
+int
+__xfs_attr_rmtval_remove(
+ struct xfs_da_args *args)
+{
+ int error, done;
+
+ /*
+ * Unmap value blocks for this attr.
+ */
+ error = xfs_bunmapi(args->trans, args->dp, args->rmtblkno,
+ args->rmtblkcnt, XFS_BMAPI_ATTRFORK, 1, &done);
+ if (error)
+ return error;
+
+ error = xfs_defer_finish(&args->trans);
+ if (error)
+ return error;
+
+ if (!done)
+ return -EAGAIN;
+
+ return error;
+}
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 3616e88..9eee615 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_remove(struct xfs_da_args *args);
#endif /* __XFS_ATTR_REMOTE_H__ */
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 11/25] xfs: Pull up xfs_attr_rmtval_invalidate
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (9 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 10/25] xfs: Refactor xfs_attr_rmtval_remove Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 12/25] xfs: Add helper function xfs_attr_node_shrink Allison Collins
` (13 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch pulls xfs_attr_rmtval_invalidate out of
xfs_attr_rmtval_remove and into the calling functions. Eventually
__xfs_attr_rmtval_remove will replace xfs_attr_rmtval_remove when we
introduce delayed attributes. These functions are exepcted to return
-EAGAIN when they need a new transaction. Because the invalidate does
not need a new transaction, we need to separate it from the rest of the
function that does. This will enable __xfs_attr_rmtval_remove to
smoothly replace xfs_attr_rmtval_remove later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
fs/xfs/libxfs/xfs_attr_remote.c | 3 ---
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index df550b4..9094031 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -670,6 +670,10 @@ xfs_attr_leaf_addname(
args->rmtblkcnt = args->rmtblkcnt2;
args->rmtvaluelen = args->rmtvaluelen2;
if (args->rmtblkno) {
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
+
error = xfs_attr_rmtval_remove(args);
if (error)
return error;
@@ -1028,6 +1032,10 @@ xfs_attr_node_addname(
args->rmtblkcnt = args->rmtblkcnt2;
args->rmtvaluelen = args->rmtvaluelen2;
if (args->rmtblkno) {
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
+
error = xfs_attr_rmtval_remove(args);
if (error)
return error;
@@ -1153,6 +1161,10 @@ xfs_attr_node_removename(
if (error)
goto out;
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
+
error = xfs_attr_rmtval_remove(args);
if (error)
goto out;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 9b4c173..85dca51 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -685,9 +685,6 @@ xfs_attr_rmtval_remove(
trace_xfs_attr_rmtval_remove(args);
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
/*
* Keep de-allocating extents until the remote-value region is gone.
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 12/25] xfs: Add helper function xfs_attr_node_shrink
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (10 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 11/25] xfs: Pull up xfs_attr_rmtval_invalidate Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls Allison Collins
` (12 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch adds a new helper function xfs_attr_node_shrink used to
shrink an attr name into an inode if it is small enough. This helps to
modularize the greater calling function xfs_attr_node_removename.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 68 ++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 9094031..4eff875 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1109,6 +1109,45 @@ xfs_attr_node_addname(
}
/*
+ * Shrink an attribute from leaf to shortform
+ */
+STATIC int
+xfs_attr_node_shrink(
+ struct xfs_da_args *args,
+ struct xfs_da_state *state)
+{
+ struct xfs_inode *dp = args->dp;
+ int error, forkoff;
+ struct xfs_buf *bp;
+
+ /*
+ * Have to get rid of the copy of this dabuf in the state.
+ */
+ ASSERT(state->path.active == 1);
+ ASSERT(state->path.blk[0].bp);
+ state->path.blk[0].bp = NULL;
+
+ error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
+ if (error)
+ return error;
+
+ forkoff = xfs_attr_shortform_allfit(bp, dp);
+ if (forkoff) {
+ error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
+ /* bp is gone due to xfs_da_shrink_inode */
+ if (error)
+ return error;
+
+ error = xfs_defer_finish(&args->trans);
+ if (error)
+ return error;
+ } else
+ xfs_trans_brelse(args->trans, bp);
+
+ return 0;
+}
+
+/*
* Remove a name from a B-tree attribute list.
*
* This will involve walking down the Btree, and may involve joining
@@ -1121,8 +1160,7 @@ xfs_attr_node_removename(
{
struct xfs_da_state *state;
struct xfs_da_state_blk *blk;
- struct xfs_buf *bp;
- int retval, error, forkoff;
+ int retval, error;
struct xfs_inode *dp = args->dp;
trace_xfs_attr_node_removename(args);
@@ -1207,30 +1245,8 @@ xfs_attr_node_removename(
/*
* If the result is small enough, push it all into the inode.
*/
- if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
- /*
- * Have to get rid of the copy of this dabuf in the state.
- */
- ASSERT(state->path.active == 1);
- ASSERT(state->path.blk[0].bp);
- state->path.blk[0].bp = NULL;
-
- error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
- if (error)
- goto out;
-
- if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
- error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
- /* bp is gone due to xfs_da_shrink_inode */
- if (error)
- goto out;
- error = xfs_defer_finish(&args->trans);
- if (error)
- goto out;
- } else
- xfs_trans_brelse(args->trans, bp);
- }
- error = 0;
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
+ error = xfs_attr_node_shrink(args, state);
out:
if (state)
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (11 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 12/25] xfs: Add helper function xfs_attr_node_shrink Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-07-08 12:41 ` Brian Foster
2020-06-25 23:30 ` [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename Allison Collins
` (11 subsequent siblings)
24 siblings, 1 reply; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
needed. If they are the last operations executed in these functions, and
no further changes are made, then higher level routines will roll or
commit the tranactions.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 61 ++++++------------------------------------------
1 file changed, 7 insertions(+), 54 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4eff875..1a78023 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -693,34 +693,15 @@ xfs_attr_leaf_addname(
/*
* If the result is small enough, shrink it all into the inode.
*/
- if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
+ forkoff = xfs_attr_shortform_allfit(bp, dp);
+ if (forkoff)
error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
- if (error)
- return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
- }
-
- /*
- * Commit the remove and start the next trans in series.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
-
} else if (args->rmtblkno > 0) {
/*
* Added a "remote" value, just clear the incomplete flag.
*/
error = xfs_attr3_leaf_clearflag(args);
- if (error)
- return error;
-
- /*
- * Commit the flag value change and start the next trans in
- * series.
- */
- error = xfs_trans_roll_inode(&args->trans, args->dp);
}
return error;
}
@@ -780,15 +761,11 @@ xfs_attr_leaf_removename(
/*
* If the result is small enough, shrink it all into the inode.
*/
- if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
- error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
+ forkoff = xfs_attr_shortform_allfit(bp, dp);
+ if (forkoff)
+ return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
- if (error)
- return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
- }
+
return 0;
}
@@ -1070,18 +1047,8 @@ xfs_attr_node_addname(
error = xfs_da3_join(state);
if (error)
goto out;
- error = xfs_defer_finish(&args->trans);
- if (error)
- goto out;
}
- /*
- * Commit and start the next trans in the chain.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- goto out;
-
} else if (args->rmtblkno > 0) {
/*
* Added a "remote" value, just clear the incomplete flag.
@@ -1089,14 +1056,6 @@ xfs_attr_node_addname(
error = xfs_attr3_leaf_clearflag(args);
if (error)
goto out;
-
- /*
- * Commit the flag value change and start the next trans in
- * series.
- */
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- goto out;
}
retval = error = 0;
@@ -1135,16 +1094,10 @@ xfs_attr_node_shrink(
if (forkoff) {
error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
- if (error)
- return error;
-
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
} else
xfs_trans_brelse(args->trans, bp);
- return 0;
+ return error;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls
2020-06-25 23:30 ` [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls Allison Collins
@ 2020-07-08 12:41 ` Brian Foster
2020-07-09 22:01 ` Allison Collins
0 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-07-08 12:41 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
On Thu, Jun 25, 2020 at 04:30:06PM -0700, Allison Collins wrote:
> Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
> needed. If they are the last operations executed in these functions, and
> no further changes are made, then higher level routines will roll or
> commit the tranactions.
transactions
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
This one LGTM now:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr.c | 61 ++++++------------------------------------------
> 1 file changed, 7 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4eff875..1a78023 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -693,34 +693,15 @@ xfs_attr_leaf_addname(
> /*
> * If the result is small enough, shrink it all into the inode.
> */
> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
> + forkoff = xfs_attr_shortform_allfit(bp, dp);
> + if (forkoff)
> error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> /* bp is gone due to xfs_da_shrink_inode */
> - if (error)
> - return error;
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - return error;
> - }
> -
> - /*
> - * Commit the remove and start the next trans in series.
> - */
> - error = xfs_trans_roll_inode(&args->trans, dp);
> -
> } else if (args->rmtblkno > 0) {
> /*
> * Added a "remote" value, just clear the incomplete flag.
> */
> error = xfs_attr3_leaf_clearflag(args);
> - if (error)
> - return error;
> -
> - /*
> - * Commit the flag value change and start the next trans in
> - * series.
> - */
> - error = xfs_trans_roll_inode(&args->trans, args->dp);
> }
> return error;
> }
> @@ -780,15 +761,11 @@ xfs_attr_leaf_removename(
> /*
> * If the result is small enough, shrink it all into the inode.
> */
> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> + forkoff = xfs_attr_shortform_allfit(bp, dp);
> + if (forkoff)
> + return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> /* bp is gone due to xfs_da_shrink_inode */
> - if (error)
> - return error;
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - return error;
> - }
> +
> return 0;
> }
>
> @@ -1070,18 +1047,8 @@ xfs_attr_node_addname(
> error = xfs_da3_join(state);
> if (error)
> goto out;
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - goto out;
> }
>
> - /*
> - * Commit and start the next trans in the chain.
> - */
> - error = xfs_trans_roll_inode(&args->trans, dp);
> - if (error)
> - goto out;
> -
> } else if (args->rmtblkno > 0) {
> /*
> * Added a "remote" value, just clear the incomplete flag.
> @@ -1089,14 +1056,6 @@ xfs_attr_node_addname(
> error = xfs_attr3_leaf_clearflag(args);
> if (error)
> goto out;
> -
> - /*
> - * Commit the flag value change and start the next trans in
> - * series.
> - */
> - error = xfs_trans_roll_inode(&args->trans, args->dp);
> - if (error)
> - goto out;
> }
> retval = error = 0;
>
> @@ -1135,16 +1094,10 @@ xfs_attr_node_shrink(
> if (forkoff) {
> error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> /* bp is gone due to xfs_da_shrink_inode */
> - if (error)
> - return error;
> -
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - return error;
> } else
> xfs_trans_brelse(args->trans, bp);
>
> - return 0;
> + return error;
> }
>
> /*
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls
2020-07-08 12:41 ` Brian Foster
@ 2020-07-09 22:01 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-07-09 22:01 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 7/8/20 5:41 AM, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 04:30:06PM -0700, Allison Collins wrote:
>> Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
>> needed. If they are the last operations executed in these functions, and
>> no further changes are made, then higher level routines will roll or
>> commit the tranactions.
>
> transactions
will fix
>
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>
> This one LGTM now:
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Great, thanks!
Allison
>
>> fs/xfs/libxfs/xfs_attr.c | 61 ++++++------------------------------------------
>> 1 file changed, 7 insertions(+), 54 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4eff875..1a78023 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -693,34 +693,15 @@ xfs_attr_leaf_addname(
>> /*
>> * If the result is small enough, shrink it all into the inode.
>> */
>> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
>> + forkoff = xfs_attr_shortform_allfit(bp, dp);
>> + if (forkoff)
>> error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - return error;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> - }
>> -
>> - /*
>> - * Commit the remove and start the next trans in series.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> -
>> } else if (args->rmtblkno > 0) {
>> /*
>> * Added a "remote" value, just clear the incomplete flag.
>> */
>> error = xfs_attr3_leaf_clearflag(args);
>> - if (error)
>> - return error;
>> -
>> - /*
>> - * Commit the flag value change and start the next trans in
>> - * series.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> }
>> return error;
>> }
>> @@ -780,15 +761,11 @@ xfs_attr_leaf_removename(
>> /*
>> * If the result is small enough, shrink it all into the inode.
>> */
>> - if ((forkoff = xfs_attr_shortform_allfit(bp, dp))) {
>> - error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> + forkoff = xfs_attr_shortform_allfit(bp, dp);
>> + if (forkoff)
>> + return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - return error;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> - }
>> +
>> return 0;
>> }
>>
>> @@ -1070,18 +1047,8 @@ xfs_attr_node_addname(
>> error = xfs_da3_join(state);
>> if (error)
>> goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> }
>>
>> - /*
>> - * Commit and start the next trans in the chain.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - goto out;
>> -
>> } else if (args->rmtblkno > 0) {
>> /*
>> * Added a "remote" value, just clear the incomplete flag.
>> @@ -1089,14 +1056,6 @@ xfs_attr_node_addname(
>> error = xfs_attr3_leaf_clearflag(args);
>> if (error)
>> goto out;
>> -
>> - /*
>> - * Commit the flag value change and start the next trans in
>> - * series.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> - if (error)
>> - goto out;
>> }
>> retval = error = 0;
>>
>> @@ -1135,16 +1094,10 @@ xfs_attr_node_shrink(
>> if (forkoff) {
>> error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
>> /* bp is gone due to xfs_da_shrink_inode */
>> - if (error)
>> - return error;
>> -
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> } else
>> xfs_trans_brelse(args->trans, bp);
>>
>> - return 0;
>> + return error;
>> }
>>
>> /*
>> --
>> 2.7.4
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (12 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 13/25] xfs: Remove unneeded xfs_trans_roll_inode calls Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-07-08 12:42 ` Brian Foster
2020-06-25 23:30 ` [PATCH v10 15/25] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform Allison Collins
` (10 subsequent siblings)
24 siblings, 1 reply; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
The xfs_trans_roll in _removename is not needed because invalidating
blocks is an incore-only change. This is analogous to the non-remote
remove case where an entry is removed and a potential dabtree join
occurs under the same transaction.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1a78023..f1becca 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1148,10 +1148,6 @@ xfs_attr_node_removename(
if (error)
goto out;
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- goto out;
-
error = xfs_attr_rmtval_invalidate(args);
if (error)
return error;
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename
2020-06-25 23:30 ` [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename Allison Collins
@ 2020-07-08 12:42 ` Brian Foster
2020-07-09 22:01 ` Allison Collins
0 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-07-08 12:42 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote:
> The xfs_trans_roll in _removename is not needed because invalidating
> blocks is an incore-only change. This is analogous to the non-remote
> remove case where an entry is removed and a potential dabtree join
> occurs under the same transaction.
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
Ok, but I think we should be a bit more descriptive in the commit log so
the reasoning is available for historical reference. For example:
"A transaction roll is not necessary immediately after setting the
INCOMPLETE flag when removing a node xattr entry with remote value
blocks. The remote block invalidation that immediately follows setting
the flag is an in-core only change. The next step after that is to start
unmapping the remote blocks from the attr fork, but the xattr remove
transaction reservation includes reservation for full tree splits of the
dabtree and bmap tree. The remote block unmap code will roll the
transaction as extents are unmapped and freed."
With something like that in place:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1a78023..f1becca 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1148,10 +1148,6 @@ xfs_attr_node_removename(
> if (error)
> goto out;
>
> - error = xfs_trans_roll_inode(&args->trans, args->dp);
> - if (error)
> - goto out;
> -
> error = xfs_attr_rmtval_invalidate(args);
> if (error)
> return error;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename
2020-07-08 12:42 ` Brian Foster
@ 2020-07-09 22:01 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-07-09 22:01 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 7/8/20 5:42 AM, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote:
>> The xfs_trans_roll in _removename is not needed because invalidating
>> blocks is an incore-only change. This is analogous to the non-remote
>> remove case where an entry is removed and a potential dabtree join
>> occurs under the same transaction.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>
> Ok, but I think we should be a bit more descriptive in the commit log so
> the reasoning is available for historical reference. For example:
>
> "A transaction roll is not necessary immediately after setting the
> INCOMPLETE flag when removing a node xattr entry with remote value
> blocks. The remote block invalidation that immediately follows setting
> the flag is an in-core only change. The next step after that is to start
> unmapping the remote blocks from the attr fork, but the xattr remove
> transaction reservation includes reservation for full tree splits of the
> dabtree and bmap tree. The remote block unmap code will roll the
> transaction as extents are unmapped and freed."
Ok, that is a lot more detailed.
>
> With something like that in place:
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Sure, will update. Thanks!
Allison
>
>> fs/xfs/libxfs/xfs_attr.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 1a78023..f1becca 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -1148,10 +1148,6 @@ xfs_attr_node_removename(
>> if (error)
>> goto out;
>>
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> - if (error)
>> - goto out;
>> -
>> error = xfs_attr_rmtval_invalidate(args);
>> if (error)
>> return error;
>> --
>> 2.7.4
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v10 15/25] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (13 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 16/25] xfs: Add helper function xfs_attr_leaf_mark_incomplete Allison Collins
` (9 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
In this patch, we hoist code from xfs_attr_set_args into two new helpers
xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help
to simplify xfs_attr_set_args when we get into delayed attrs later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
1 file changed, 72 insertions(+), 35 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index f1becca..b828f3b 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
}
/*
+ * Check to see if the attr should be upgraded from non-existent or shortform to
+ * single-leaf-block attribute list.
+ */
+static inline bool
+xfs_attr_is_shortform(
+ struct xfs_inode *ip)
+{
+ return ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL ||
+ (ip->i_afp->if_format == XFS_DINODE_FMT_EXTENTS &&
+ ip->i_afp->if_nextents == 0);
+}
+
+/*
+ * Attempts to set an attr in shortform, or converts short form to leaf form if
+ * there is not enough room. If the attr is set, the transaction is committed
+ * and set to NULL.
+ */
+STATIC int
+xfs_attr_set_shortform(
+ struct xfs_da_args *args,
+ struct xfs_buf **leaf_bp)
+{
+ struct xfs_inode *dp = args->dp;
+ int error, error2 = 0;
+
+ /*
+ * Try to add the attr to the attribute list in the inode.
+ */
+ error = xfs_attr_try_sf_addname(dp, args);
+ if (error != -ENOSPC) {
+ error2 = xfs_trans_commit(args->trans);
+ args->trans = NULL;
+ return error ? error : error2;
+ }
+ /*
+ * It won't fit in the shortform, transform to a leaf block. GROT:
+ * another possible req'mt for a double-split btree op.
+ */
+ error = xfs_attr_shortform_to_leaf(args, leaf_bp);
+ if (error)
+ return error;
+
+ /*
+ * Prevent the leaf buffer from being unlocked so that a concurrent AIL
+ * push cannot grab the half-baked leaf buffer and run into problems
+ * with the write verifier. Once we're done rolling the transaction we
+ * can release the hold and add the attr to the leaf.
+ */
+ xfs_trans_bhold(args->trans, *leaf_bp);
+ error = xfs_defer_finish(&args->trans);
+ xfs_trans_bhold_release(args->trans, *leaf_bp);
+ if (error) {
+ xfs_trans_brelse(args->trans, *leaf_bp);
+ return error;
+ }
+
+ return 0;
+}
+
+/*
* Set the attribute specified in @args.
*/
int
@@ -212,48 +272,25 @@ xfs_attr_set_args(
{
struct xfs_inode *dp = args->dp;
struct xfs_buf *leaf_bp = NULL;
- int error, error2 = 0;
+ int error = 0;
/*
- * If the attribute list is non-existent or a shortform list,
- * upgrade it to a single-leaf-block attribute list.
+ * If the attribute list is already in leaf format, jump straight to
+ * leaf handling. Otherwise, try to add the attribute to the shortform
+ * list; if there's no room then convert the list to leaf format and try
+ * again.
*/
- if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL ||
- (dp->i_afp->if_format == XFS_DINODE_FMT_EXTENTS &&
- dp->i_afp->if_nextents == 0)) {
+ if (xfs_attr_is_shortform(dp)) {
/*
- * Try to add the attr to the attribute list in the inode.
+ * If the attr was successfully set in shortform, the
+ * transaction is committed and set to NULL. Otherwise, is it
+ * converted from shortform to leaf, and the transaction is
+ * retained.
*/
- error = xfs_attr_try_sf_addname(dp, args);
- if (error != -ENOSPC) {
- error2 = xfs_trans_commit(args->trans);
- args->trans = NULL;
- return error ? error : error2;
- }
-
- /*
- * It won't fit in the shortform, transform to a leaf block.
- * GROT: another possible req'mt for a double-split btree op.
- */
- error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
- if (error)
- return error;
-
- /*
- * Prevent the leaf buffer from being unlocked so that a
- * concurrent AIL push cannot grab the half-baked leaf
- * buffer and run into problems with the write verifier.
- * Once we're done rolling the transaction we can release
- * the hold and add the attr to the leaf.
- */
- xfs_trans_bhold(args->trans, leaf_bp);
- error = xfs_defer_finish(&args->trans);
- xfs_trans_bhold_release(args->trans, leaf_bp);
- if (error) {
- xfs_trans_brelse(args->trans, leaf_bp);
+ error = xfs_attr_set_shortform(args, &leaf_bp);
+ if (error || !args->trans)
return error;
- }
}
if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 16/25] xfs: Add helper function xfs_attr_leaf_mark_incomplete
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (14 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 15/25] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 17/25] xfs: Add remote block helper functions Allison Collins
` (8 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch helps to simplify xfs_attr_node_removename by modularizing
the code around the transactions into helper functions. This will make
the function easier to follow when we introduce delayed attributes.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b828f3b..229940c 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1138,6 +1138,32 @@ xfs_attr_node_shrink(
}
/*
+ * Mark an attribute entry INCOMPLETE and save pointers to the relevant buffers
+ * for later deletion of the entry.
+ */
+STATIC int
+xfs_attr_leaf_mark_incomplete(
+ struct xfs_da_args *args,
+ struct xfs_da_state *state)
+{
+ int error;
+
+ /*
+ * Fill in disk block numbers in the state structure
+ * so that we can get the buffers back after we commit
+ * several transactions in the following calls.
+ */
+ error = xfs_attr_fillstate(state);
+ if (error)
+ return error;
+
+ /*
+ * Mark the attribute as INCOMPLETE
+ */
+ return xfs_attr3_leaf_setflag(args);
+}
+
+/*
* Remove a name from a B-tree attribute list.
*
* This will involve walking down the Btree, and may involve joining
@@ -1168,20 +1194,7 @@ xfs_attr_node_removename(
ASSERT(blk->bp != NULL);
ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
if (args->rmtblkno > 0) {
- /*
- * Fill in disk block numbers in the state structure
- * so that we can get the buffers back after we commit
- * several transactions in the following calls.
- */
- error = xfs_attr_fillstate(state);
- if (error)
- goto out;
-
- /*
- * Mark the attribute as INCOMPLETE, then bunmapi() the
- * remote value.
- */
- error = xfs_attr3_leaf_setflag(args);
+ error = xfs_attr_leaf_mark_incomplete(args, state);
if (error)
goto out;
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 17/25] xfs: Add remote block helper functions
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (15 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 16/25] xfs: Add helper function xfs_attr_leaf_mark_incomplete Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 18/25] xfs: Add helper function xfs_attr_node_removename_setup Allison Collins
` (7 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch adds two new helper functions xfs_attr_store_rmt_blk and
xfs_attr_restore_rmt_blk. These two helpers assist to remove redundant
code associated with storing and retrieving remote blocks during the
attr set operations.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 229940c..ff7523f 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -564,6 +564,30 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
* External routines when attribute list is one block
*========================================================================*/
+/* Store info about a remote block */
+STATIC void
+xfs_attr_save_rmt_blk(
+ struct xfs_da_args *args)
+{
+ args->blkno2 = args->blkno;
+ args->index2 = args->index;
+ args->rmtblkno2 = args->rmtblkno;
+ args->rmtblkcnt2 = args->rmtblkcnt;
+ args->rmtvaluelen2 = args->rmtvaluelen;
+}
+
+/* Set stored info about a remote block */
+STATIC void
+xfs_attr_restore_rmt_blk(
+ struct xfs_da_args *args)
+{
+ args->blkno = args->blkno2;
+ args->index = args->index2;
+ args->rmtblkno = args->rmtblkno2;
+ args->rmtblkcnt = args->rmtblkcnt2;
+ args->rmtvaluelen = args->rmtvaluelen2;
+}
+
/*
* Tries to add an attribute to an inode in leaf form
*
@@ -598,11 +622,7 @@ xfs_attr_leaf_try_add(
/* save the attribute state for later removal*/
args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */
- args->blkno2 = args->blkno; /* set 2nd entry info*/
- args->index2 = args->index;
- args->rmtblkno2 = args->rmtblkno;
- args->rmtblkcnt2 = args->rmtblkcnt;
- args->rmtvaluelen2 = args->rmtvaluelen;
+ xfs_attr_save_rmt_blk(args);
/*
* clear the remote attr state now that it is saved so that the
@@ -701,11 +721,8 @@ xfs_attr_leaf_addname(
* Dismantle the "old" attribute/value pair by removing
* a "remote" value (if it exists).
*/
- args->index = args->index2;
- args->blkno = args->blkno2;
- args->rmtblkno = args->rmtblkno2;
- args->rmtblkcnt = args->rmtblkcnt2;
- args->rmtvaluelen = args->rmtvaluelen2;
+ xfs_attr_restore_rmt_blk(args);
+
if (args->rmtblkno) {
error = xfs_attr_rmtval_invalidate(args);
if (error)
@@ -924,11 +941,7 @@ xfs_attr_node_addname(
/* save the attribute state for later removal*/
args->op_flags |= XFS_DA_OP_RENAME; /* atomic rename op */
- args->blkno2 = args->blkno; /* set 2nd entry info*/
- args->index2 = args->index;
- args->rmtblkno2 = args->rmtblkno;
- args->rmtblkcnt2 = args->rmtblkcnt;
- args->rmtvaluelen2 = args->rmtvaluelen;
+ xfs_attr_save_rmt_blk(args);
/*
* clear the remote attr state now that it is saved so that the
@@ -1040,11 +1053,8 @@ xfs_attr_node_addname(
* Dismantle the "old" attribute/value pair by removing
* a "remote" value (if it exists).
*/
- args->index = args->index2;
- args->blkno = args->blkno2;
- args->rmtblkno = args->rmtblkno2;
- args->rmtblkcnt = args->rmtblkcnt2;
- args->rmtvaluelen = args->rmtvaluelen2;
+ xfs_attr_restore_rmt_blk(args);
+
if (args->rmtblkno) {
error = xfs_attr_rmtval_invalidate(args);
if (error)
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 18/25] xfs: Add helper function xfs_attr_node_removename_setup
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (16 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 17/25] xfs: Add remote block helper functions Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 19/25] xfs: Add helper function xfs_attr_node_removename_rmt Allison Collins
` (6 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch adds a new helper function xfs_attr_node_removename_setup.
This will help modularize xfs_attr_node_removename when we add delay
ready attributes later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 47 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index ff7523f..f84c163 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1174,6 +1174,38 @@ xfs_attr_leaf_mark_incomplete(
}
/*
+ * Initial setup for xfs_attr_node_removename. Make sure the attr is there and
+ * the blocks are valid. Attr keys with remote blocks will be marked
+ * incomplete.
+ */
+STATIC
+int xfs_attr_node_removename_setup(
+ struct xfs_da_args *args,
+ struct xfs_da_state **state)
+{
+ int error;
+ struct xfs_da_state_blk *blk;
+
+ error = xfs_attr_node_hasname(args, state);
+ if (error != -EEXIST)
+ return error;
+
+ blk = &(*state)->path.blk[(*state)->path.active - 1];
+ ASSERT(blk->bp != NULL);
+ ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
+
+ if (args->rmtblkno > 0) {
+ error = xfs_attr_leaf_mark_incomplete(args, *state);
+ if (error)
+ return error;
+
+ return xfs_attr_rmtval_invalidate(args);
+ }
+
+ return 0;
+}
+
+/*
* Remove a name from a B-tree attribute list.
*
* This will involve walking down the Btree, and may involve joining
@@ -1191,8 +1223,8 @@ xfs_attr_node_removename(
trace_xfs_attr_node_removename(args);
- error = xfs_attr_node_hasname(args, &state);
- if (error != -EEXIST)
+ error = xfs_attr_node_removename_setup(args, &state);
+ if (error)
goto out;
/*
@@ -1200,18 +1232,7 @@ xfs_attr_node_removename(
* This is done before we remove the attribute so that we don't
* overflow the maximum size of a transaction and/or hit a deadlock.
*/
- blk = &state->path.blk[ state->path.active-1 ];
- ASSERT(blk->bp != NULL);
- ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
if (args->rmtblkno > 0) {
- error = xfs_attr_leaf_mark_incomplete(args, state);
- if (error)
- goto out;
-
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
-
error = xfs_attr_rmtval_remove(args);
if (error)
goto out;
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 19/25] xfs: Add helper function xfs_attr_node_removename_rmt
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (17 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 18/25] xfs: Add helper function xfs_attr_node_removename_setup Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 20/25] xfs: Simplify xfs_attr_leaf_addname Allison Collins
` (5 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch adds another new helper function
xfs_attr_node_removename_rmt. This will also help modularize
xfs_attr_node_removename when we add delay ready attributes later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index f84c163..f993af5 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1205,6 +1205,24 @@ int xfs_attr_node_removename_setup(
return 0;
}
+STATIC int
+xfs_attr_node_remove_rmt(
+ struct xfs_da_args *args,
+ struct xfs_da_state *state)
+{
+ int error = 0;
+
+ error = xfs_attr_rmtval_remove(args);
+ if (error)
+ return error;
+
+ /*
+ * Refill the state structure with buffers, the prior calls released our
+ * buffers.
+ */
+ return xfs_attr_refillstate(state);
+}
+
/*
* Remove a name from a B-tree attribute list.
*
@@ -1233,15 +1251,7 @@ xfs_attr_node_removename(
* overflow the maximum size of a transaction and/or hit a deadlock.
*/
if (args->rmtblkno > 0) {
- error = xfs_attr_rmtval_remove(args);
- if (error)
- goto out;
-
- /*
- * Refill the state structure with buffers, the prior calls
- * released our buffers.
- */
- error = xfs_attr_refillstate(state);
+ error = xfs_attr_node_remove_rmt(args, state);
if (error)
goto out;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 20/25] xfs: Simplify xfs_attr_leaf_addname
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (18 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 19/25] xfs: Add helper function xfs_attr_node_removename_rmt Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 21/25] xfs: Simplify xfs_attr_node_addname Allison Collins
` (4 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Invert the rename logic in xfs_attr_leaf_addname to simplify the
delayed attr logic later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 107 ++++++++++++++++++++++++-----------------------
1 file changed, 55 insertions(+), 52 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index f993af5..ca1e851 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -695,68 +695,71 @@ xfs_attr_leaf_addname(
return error;
}
- /*
- * If this is an atomic rename operation, we must "flip" the
- * incomplete flags on the "new" and "old" attribute/value pairs
- * so that one disappears and one appears atomically. Then we
- * must remove the "old" attribute/value pair.
- */
- if (args->op_flags & XFS_DA_OP_RENAME) {
+ if (!(args->op_flags & XFS_DA_OP_RENAME)) {
/*
- * In a separate transaction, set the incomplete flag on the
- * "old" attr and clear the incomplete flag on the "new" attr.
- */
- error = xfs_attr3_leaf_flipflags(args);
- if (error)
- return error;
- /*
- * Commit the flag value change and start the next trans in
- * series.
+ * Added a "remote" value, just clear the incomplete flag.
*/
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- return error;
+ if (args->rmtblkno > 0)
+ error = xfs_attr3_leaf_clearflag(args);
- /*
- * Dismantle the "old" attribute/value pair by removing
- * a "remote" value (if it exists).
- */
- xfs_attr_restore_rmt_blk(args);
+ return error;
+ }
- if (args->rmtblkno) {
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
+ /*
+ * If this is an atomic rename operation, we must "flip" the incomplete
+ * flags on the "new" and "old" attribute/value pairs so that one
+ * disappears and one appears atomically. Then we must remove the "old"
+ * attribute/value pair.
+ *
+ * In a separate transaction, set the incomplete flag on the "old" attr
+ * and clear the incomplete flag on the "new" attr.
+ */
- error = xfs_attr_rmtval_remove(args);
- if (error)
- return error;
- }
+ error = xfs_attr3_leaf_flipflags(args);
+ if (error)
+ return error;
+ /*
+ * Commit the flag value change and start the next trans in series.
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ return error;
- /*
- * Read in the block containing the "old" attr, then
- * remove the "old" attr from that block (neat, huh!)
- */
- error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
- &bp);
+ /*
+ * Dismantle the "old" attribute/value pair by removing a "remote" value
+ * (if it exists).
+ */
+ xfs_attr_restore_rmt_blk(args);
+
+ if (args->rmtblkno) {
+ error = xfs_attr_rmtval_invalidate(args);
if (error)
return error;
- xfs_attr3_leaf_remove(bp, args);
-
- /*
- * If the result is small enough, shrink it all into the inode.
- */
- forkoff = xfs_attr_shortform_allfit(bp, dp);
- if (forkoff)
- error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
- /* bp is gone due to xfs_da_shrink_inode */
- } else if (args->rmtblkno > 0) {
- /*
- * Added a "remote" value, just clear the incomplete flag.
- */
- error = xfs_attr3_leaf_clearflag(args);
+ error = xfs_attr_rmtval_remove(args);
+ if (error)
+ return error;
}
+
+ /*
+ * Read in the block containing the "old" attr, then remove the "old"
+ * attr from that block (neat, huh!)
+ */
+ error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
+ &bp);
+ if (error)
+ return error;
+
+ xfs_attr3_leaf_remove(bp, args);
+
+ /*
+ * If the result is small enough, shrink it all into the inode.
+ */
+ forkoff = xfs_attr_shortform_allfit(bp, dp);
+ if (forkoff)
+ error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
+ /* bp is gone due to xfs_da_shrink_inode */
+
return error;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 21/25] xfs: Simplify xfs_attr_node_addname
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (19 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 20/25] xfs: Simplify xfs_attr_leaf_addname Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 22/25] xfs: Lift -ENOSPC handler from xfs_attr_leaf_addname Allison Collins
` (3 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Invert the rename logic in xfs_attr_node_addname to simplify the
delayed attr logic later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 125 +++++++++++++++++++++++------------------------
1 file changed, 61 insertions(+), 64 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index ca1e851..e618b09 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1030,80 +1030,77 @@ xfs_attr_node_addname(
return error;
}
- /*
- * If this is an atomic rename operation, we must "flip" the
- * incomplete flags on the "new" and "old" attribute/value pairs
- * so that one disappears and one appears atomically. Then we
- * must remove the "old" attribute/value pair.
- */
- if (args->op_flags & XFS_DA_OP_RENAME) {
- /*
- * In a separate transaction, set the incomplete flag on the
- * "old" attr and clear the incomplete flag on the "new" attr.
- */
- error = xfs_attr3_leaf_flipflags(args);
- if (error)
- goto out;
+ if (!(args->op_flags & XFS_DA_OP_RENAME)) {
/*
- * Commit the flag value change and start the next trans in
- * series
+ * Added a "remote" value, just clear the incomplete flag.
*/
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- goto out;
+ if (args->rmtblkno > 0)
+ error = xfs_attr3_leaf_clearflag(args);
+ retval = error;
+ goto out;
+ }
- /*
- * Dismantle the "old" attribute/value pair by removing
- * a "remote" value (if it exists).
- */
- xfs_attr_restore_rmt_blk(args);
+ /*
+ * If this is an atomic rename operation, we must "flip" the incomplete
+ * flags on the "new" and "old" attribute/value pairs so that one
+ * disappears and one appears atomically. Then we must remove the "old"
+ * attribute/value pair.
+ *
+ * In a separate transaction, set the incomplete flag on the "old" attr
+ * and clear the incomplete flag on the "new" attr.
+ */
+ error = xfs_attr3_leaf_flipflags(args);
+ if (error)
+ goto out;
+ /*
+ * Commit the flag value change and start the next trans in series
+ */
+ error = xfs_trans_roll_inode(&args->trans, args->dp);
+ if (error)
+ goto out;
- if (args->rmtblkno) {
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
+ /*
+ * Dismantle the "old" attribute/value pair by removing a "remote" value
+ * (if it exists).
+ */
+ xfs_attr_restore_rmt_blk(args);
- error = xfs_attr_rmtval_remove(args);
- if (error)
- return error;
- }
+ if (args->rmtblkno) {
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
- /*
- * Re-find the "old" attribute entry after any split ops.
- * The INCOMPLETE flag means that we will find the "old"
- * attr, not the "new" one.
- */
- args->attr_filter |= XFS_ATTR_INCOMPLETE;
- state = xfs_da_state_alloc();
- state->args = args;
- state->mp = mp;
- state->inleaf = 0;
- error = xfs_da3_node_lookup_int(state, &retval);
+ error = xfs_attr_rmtval_remove(args);
if (error)
- goto out;
+ return error;
+ }
- /*
- * Remove the name and update the hashvals in the tree.
- */
- blk = &state->path.blk[ state->path.active-1 ];
- ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
- error = xfs_attr3_leaf_remove(blk->bp, args);
- xfs_da3_fixhashpath(state, &state->path);
+ /*
+ * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
+ * flag means that we will find the "old" attr, not the "new" one.
+ */
+ args->attr_filter |= XFS_ATTR_INCOMPLETE;
+ state = xfs_da_state_alloc();
+ state->args = args;
+ state->mp = mp;
+ state->inleaf = 0;
+ error = xfs_da3_node_lookup_int(state, &retval);
+ if (error)
+ goto out;
- /*
- * Check to see if the tree needs to be collapsed.
- */
- if (retval && (state->path.active > 1)) {
- error = xfs_da3_join(state);
- if (error)
- goto out;
- }
+ /*
+ * Remove the name and update the hashvals in the tree.
+ */
+ blk = &state->path.blk[state->path.active-1];
+ ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
+ error = xfs_attr3_leaf_remove(blk->bp, args);
+ xfs_da3_fixhashpath(state, &state->path);
- } else if (args->rmtblkno > 0) {
- /*
- * Added a "remote" value, just clear the incomplete flag.
- */
- error = xfs_attr3_leaf_clearflag(args);
+ /*
+ * Check to see if the tree needs to be collapsed.
+ */
+ if (retval && (state->path.active > 1)) {
+ error = xfs_da3_join(state);
if (error)
goto out;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 22/25] xfs: Lift -ENOSPC handler from xfs_attr_leaf_addname
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (20 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 21/25] xfs: Simplify xfs_attr_node_addname Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 23/25] xfs: Add delay ready attr remove routines Allison Collins
` (2 subsequent siblings)
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Lift -ENOSPC handler from xfs_attr_leaf_addname. This will help to
reorganize transitions between the attr forms later.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e618b09..4b78c86 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -299,6 +299,13 @@ xfs_attr_set_args(
return error;
/*
+ * Promote the attribute list to the Btree format.
+ */
+ error = xfs_attr3_leaf_to_node(args);
+ if (error)
+ return error;
+
+ /*
* Finish any deferred work items and roll the transaction once
* more. The goal here is to call node_addname with the inode
* and transaction in the same state (inode locked and joined,
@@ -603,7 +610,7 @@ xfs_attr_leaf_try_add(
struct xfs_da_args *args,
struct xfs_buf *bp)
{
- int retval, error;
+ int retval;
/*
* Look up the given attribute in the leaf block. Figure out if
@@ -635,20 +642,10 @@ xfs_attr_leaf_try_add(
}
/*
- * Add the attribute to the leaf block, transitioning to a Btree
- * if required.
+ * Add the attribute to the leaf block
*/
- retval = xfs_attr3_leaf_add(bp, args);
- if (retval == -ENOSPC) {
- /*
- * Promote the attribute list to the Btree format. Unless an
- * error occurs, retain the -ENOSPC retval
- */
- error = xfs_attr3_leaf_to_node(args);
- if (error)
- return error;
- }
- return retval;
+ return xfs_attr3_leaf_add(bp, args);
+
out_brelse:
xfs_trans_brelse(args->trans, bp);
return retval;
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (21 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 22/25] xfs: Lift -ENOSPC handler from xfs_attr_leaf_addname Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-07-09 13:24 ` Brian Foster
2020-06-25 23:30 ` [PATCH v10 24/25] xfs: Add delay ready attr set routines Allison Collins
2020-06-25 23:30 ` [PATCH v10 25/25] xfs: Rename __xfs_attr_rmtval_remove Allison Collins
24 siblings, 1 reply; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch modifies the attr remove routines to be delay ready. This
means they no longer roll or commit transactions, but instead return
-EAGAIN to have the calling routine roll and refresh the transaction. In
this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
uses a sort of state machine like switch to keep track of where it was
when EAGAIN was returned. xfs_attr_node_removename has also been
modified to use the switch, and a new version of xfs_attr_remove_args
consists of a simple loop to refresh the transaction until the operation
is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
transaction where ever the existing code used to.
Calls to xfs_attr_rmtval_remove are replaced with the delay ready
version __xfs_attr_rmtval_remove. We will rename
__xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
done.
xfs_attr_rmtval_remove itself is still in use by the set routines (used
during a rename). For reasons of perserving existing function, we
modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
set. Similar to how xfs_attr_remove_args does here. Once we transition
the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
used and will be removed.
This patch also adds a new struct xfs_delattr_context, which we will use
to keep track of the current state of an attribute operation. The new
xfs_delattr_state enum is used to track various operations that are in
progress so that we know not to repeat them, and resume where we left
off before EAGAIN was returned to cycle out the transaction. Other
members take the place of local variables that need to retain their
values across multiple function recalls. See xfs_attr.h for a more
detailed diagram of the states.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
fs/xfs/xfs_attr_inactive.c | 2 +-
6 files changed, 208 insertions(+), 66 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4b78c86..5c460f4 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
*/
STATIC int xfs_attr_node_get(xfs_da_args_t *args);
STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
-STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
struct xfs_da_state **state);
STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
@@ -264,6 +264,32 @@ xfs_attr_set_shortform(
}
/*
+ * Checks to see if a delayed attribute transaction should be rolled. If so,
+ * also checks for a defer finish. Transaction is finished and rolled as
+ * needed, and returns true of false if the delayed operation should continue.
+ */
+bool
+xfs_attr_roll_again(
+ struct xfs_delattr_context *dac,
+ int *error)
+{
+ struct xfs_da_args *args = dac->da_args;
+
+ if (*error != -EAGAIN)
+ return false;
+
+ if (dac->flags & XFS_DAC_DEFER_FINISH) {
+ dac->flags &= ~XFS_DAC_DEFER_FINISH;
+ *error = xfs_defer_finish(&args->trans);
+ if (*error)
+ return false;
+ }
+
+ *error = xfs_trans_roll_inode(&args->trans, args->dp);
+ return *error == 0;
+}
+
+/*
* Set the attribute specified in @args.
*/
int
@@ -364,23 +390,47 @@ xfs_has_attr(
*/
int
xfs_attr_remove_args(
- struct xfs_da_args *args)
+ struct xfs_da_args *args)
{
- struct xfs_inode *dp = args->dp;
- int error;
+ int error = 0;
+ struct xfs_delattr_context dac = {
+ .da_args = args,
+ };
+
+ do {
+ error = xfs_attr_remove_iter(&dac);
+ } while (xfs_attr_roll_again(&dac, &error));
+
+ return error;
+}
+
+/*
+ * Remove the attribute specified in @args.
+ *
+ * This function may return -EAGAIN to signal that the transaction needs to be
+ * rolled. Callers should continue calling this function until they receive a
+ * return value other than -EAGAIN.
+ */
+int
+xfs_attr_remove_iter(
+ struct xfs_delattr_context *dac)
+{
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_inode *dp = args->dp;
+
+ if (dac->dela_state == XFS_DAS_RM_SHRINK)
+ goto node;
if (!xfs_inode_hasattr(dp)) {
- error = -ENOATTR;
+ return -ENOATTR;
} else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
- error = xfs_attr_shortform_remove(args);
+ return xfs_attr_shortform_remove(args);
} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
- error = xfs_attr_leaf_removename(args);
- } else {
- error = xfs_attr_node_removename(args);
+ return xfs_attr_leaf_removename(args);
}
-
- return error;
+node:
+ return xfs_attr_node_removename(dac);
}
/*
@@ -1177,11 +1227,12 @@ xfs_attr_leaf_mark_incomplete(
*/
STATIC
int xfs_attr_node_removename_setup(
- struct xfs_da_args *args,
- struct xfs_da_state **state)
+ struct xfs_delattr_context *dac,
+ struct xfs_da_state **state)
{
- int error;
- struct xfs_da_state_blk *blk;
+ struct xfs_da_args *args = dac->da_args;
+ int error;
+ struct xfs_da_state_blk *blk;
error = xfs_attr_node_hasname(args, state);
if (error != -EEXIST)
@@ -1191,6 +1242,13 @@ int xfs_attr_node_removename_setup(
ASSERT(blk->bp != NULL);
ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
+ /*
+ * Store blk and state in the context incase we need to cycle out the
+ * transaction
+ */
+ dac->blk = blk;
+ dac->da_state = *state;
+
if (args->rmtblkno > 0) {
error = xfs_attr_leaf_mark_incomplete(args, *state);
if (error)
@@ -1203,13 +1261,16 @@ int xfs_attr_node_removename_setup(
}
STATIC int
-xfs_attr_node_remove_rmt(
- struct xfs_da_args *args,
- struct xfs_da_state *state)
+xfs_attr_node_remove_rmt (
+ struct xfs_delattr_context *dac,
+ struct xfs_da_state *state)
{
- int error = 0;
+ int error = 0;
- error = xfs_attr_rmtval_remove(args);
+ /*
+ * May return -EAGAIN to request that the caller recall this function
+ */
+ error = __xfs_attr_rmtval_remove(dac);
if (error)
return error;
@@ -1226,21 +1287,35 @@ xfs_attr_node_remove_rmt(
* This will involve walking down the Btree, and may involve joining
* leaf nodes and even joining intermediate nodes up to and including
* the root node (a special case of an intermediate node).
+ *
+ * This routine is meant to function as either an inline or delayed operation,
+ * and may return -EAGAIN when the transaction needs to be rolled. Calling
+ * functions will need to handle this, and recall the function until a
+ * successful error code is returned.
*/
STATIC int
xfs_attr_node_removename(
- struct xfs_da_args *args)
+ struct xfs_delattr_context *dac)
{
- struct xfs_da_state *state;
- struct xfs_da_state_blk *blk;
- int retval, error;
- struct xfs_inode *dp = args->dp;
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_da_state *state;
+ struct xfs_da_state_blk *blk;
+ int retval, error;
+ struct xfs_inode *dp = args->dp;
trace_xfs_attr_node_removename(args);
+ state = dac->da_state;
+ blk = dac->blk;
- error = xfs_attr_node_removename_setup(args, &state);
- if (error)
- goto out;
+ if (dac->dela_state == XFS_DAS_RM_SHRINK)
+ goto das_rm_shrink;
+
+ if ((dac->flags & XFS_DAC_NODE_RMVNAME_INIT) == 0) {
+ dac->flags |= XFS_DAC_NODE_RMVNAME_INIT;
+ error = xfs_attr_node_removename_setup(dac, &state);
+ if (error)
+ goto out;
+ }
/*
* If there is an out-of-line value, de-allocate the blocks.
@@ -1248,8 +1323,13 @@ xfs_attr_node_removename(
* overflow the maximum size of a transaction and/or hit a deadlock.
*/
if (args->rmtblkno > 0) {
- error = xfs_attr_node_remove_rmt(args, state);
- if (error)
+ /*
+ * May return -EAGAIN. Remove blocks until args->rmtblkno == 0
+ */
+ error = xfs_attr_node_remove_rmt(dac, state);
+ if (error == -EAGAIN)
+ return error;
+ else if (error)
goto out;
}
@@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
error = xfs_da3_join(state);
if (error)
goto out;
- error = xfs_defer_finish(&args->trans);
- if (error)
- goto out;
- /*
- * Commit the Btree join operation and start a new trans.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- goto out;
+
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+ dac->dela_state = XFS_DAS_RM_SHRINK;
+ return -EAGAIN;
}
+das_rm_shrink:
+
/*
* If the result is small enough, push it all into the inode.
*/
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 3e97a93..6c58792 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -74,6 +74,75 @@ struct xfs_attr_list_context {
};
+/*
+ * ========================================================================
+ * Structure used to pass context around among the delayed routines.
+ * ========================================================================
+ */
+
+/*
+ * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
+ * states indicate places where the function would return -EAGAIN, and then
+ * immediately resume from after being recalled by the calling function. States
+ * marked as a "subroutine state" indicate that they belong to a subroutine, and
+ * so the calling function needs to pass them back to that subroutine to allow
+ * it to finish where it left off. But they otherwise do not have a role in the
+ * calling function other than just passing through.
+ *
+ * xfs_attr_remove_iter()
+ * XFS_DAS_RM_SHRINK ─┐
+ * (subroutine state) │
+ * └─>xfs_attr_node_removename()
+ * │
+ * v
+ * need to
+ * shrink tree? ─n─┐
+ * │ │
+ * y │
+ * │ │
+ * v │
+ * XFS_DAS_RM_SHRINK │
+ * │ │
+ * v │
+ * done <─────┘
+ *
+ */
+
+/*
+ * Enum values for xfs_delattr_context.da_state
+ *
+ * These values are used by delayed attribute operations to keep track of where
+ * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
+ * calling function to roll the transaction, and then recall the subroutine to
+ * finish the operation. The enum is then used by the subroutine to jump back
+ * to where it was and resume executing where it left off.
+ */
+enum xfs_delattr_state {
+ /* Zero is uninitalized */
+ XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
+};
+
+/*
+ * Defines for xfs_delattr_context.flags
+ */
+#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
+#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
+
+/*
+ * Context used for keeping track of delayed attribute operations
+ */
+struct xfs_delattr_context {
+ struct xfs_da_args *da_args;
+
+ /* Used in xfs_attr_node_removename to roll through removing blocks */
+ struct xfs_da_state *da_state;
+ struct xfs_da_state_blk *blk;
+
+ /* Used to keep track of current state of delayed operation */
+ unsigned int flags;
+ enum xfs_delattr_state dela_state;
+};
+
/*========================================================================
* Function prototypes for the kernel.
*========================================================================*/
@@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
int xfs_attr_set_args(struct xfs_da_args *args);
int xfs_has_attr(struct xfs_da_args *args);
int xfs_attr_remove_args(struct xfs_da_args *args);
+int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
+bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
bool xfs_attr_namecheck(const void *name, size_t length);
+void xfs_delattr_context_init(struct xfs_delattr_context *dac,
+ struct xfs_da_args *args);
#endif /* __XFS_ATTR_H__ */
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 351351c..20521bf 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -19,8 +19,8 @@
#include "xfs_bmap_btree.h"
#include "xfs_bmap.h"
#include "xfs_attr_sf.h"
-#include "xfs_attr_remote.h"
#include "xfs_attr.h"
+#include "xfs_attr_remote.h"
#include "xfs_attr_leaf.h"
#include "xfs_error.h"
#include "xfs_trace.h"
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 85dca51..20e4605 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
*/
int
xfs_attr_rmtval_remove(
- struct xfs_da_args *args)
+ struct xfs_da_args *args)
{
- xfs_dablk_t lblkno;
- int blkcnt;
- int error = 0;
- int retval = 0;
+ xfs_dablk_t lblkno;
+ int blkcnt;
+ int error = 0;
+ struct xfs_delattr_context dac = {
+ .da_args = args,
+ };
trace_xfs_attr_rmtval_remove(args);
@@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
lblkno = args->rmtblkno;
blkcnt = args->rmtblkcnt;
do {
- retval = __xfs_attr_rmtval_remove(args);
- if (retval && retval != EAGAIN)
- return retval;
-
- /*
- * Close out trans and start the next one in the chain.
- */
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- return error;
- } while (retval == -EAGAIN);
+ error = __xfs_attr_rmtval_remove(&dac);
+ } while (xfs_attr_roll_again(&dac, &error));
- return 0;
+ return error;
}
/*
@@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
*/
int
__xfs_attr_rmtval_remove(
- struct xfs_da_args *args)
+ struct xfs_delattr_context *dac)
{
- int error, done;
+ struct xfs_da_args *args = dac->da_args;
+ int error, done;
/*
* Unmap value blocks for this attr.
@@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
if (error)
return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
-
- if (!done)
+ if (!done) {
+ dac->flags |= XFS_DAC_DEFER_FINISH;
return -EAGAIN;
+ }
return error;
}
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 9eee615..002fd30 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -14,5 +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_remove(struct xfs_da_args *args);
+int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
#endif /* __XFS_ATTR_REMOTE_H__ */
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index bfad669..aaa7e66 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -15,10 +15,10 @@
#include "xfs_da_format.h"
#include "xfs_da_btree.h"
#include "xfs_inode.h"
+#include "xfs_attr.h"
#include "xfs_attr_remote.h"
#include "xfs_trans.h"
#include "xfs_bmap.h"
-#include "xfs_attr.h"
#include "xfs_attr_leaf.h"
#include "xfs_quota.h"
#include "xfs_dir2.h"
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-06-25 23:30 ` [PATCH v10 23/25] xfs: Add delay ready attr remove routines Allison Collins
@ 2020-07-09 13:24 ` Brian Foster
2020-07-09 18:04 ` Darrick J. Wong
2020-07-09 22:02 ` Allison Collins
0 siblings, 2 replies; 37+ messages in thread
From: Brian Foster @ 2020-07-09 13:24 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
> This patch modifies the attr remove routines to be delay ready. This
> means they no longer roll or commit transactions, but instead return
> -EAGAIN to have the calling routine roll and refresh the transaction. In
> this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
> uses a sort of state machine like switch to keep track of where it was
> when EAGAIN was returned. xfs_attr_node_removename has also been
> modified to use the switch, and a new version of xfs_attr_remove_args
> consists of a simple loop to refresh the transaction until the operation
> is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
> transaction where ever the existing code used to.
>
> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> version __xfs_attr_rmtval_remove. We will rename
> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> done.
>
> xfs_attr_rmtval_remove itself is still in use by the set routines (used
> during a rename). For reasons of perserving existing function, we
> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> set. Similar to how xfs_attr_remove_args does here. Once we transition
> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> used and will be removed.
>
> This patch also adds a new struct xfs_delattr_context, which we will use
> to keep track of the current state of an attribute operation. The new
> xfs_delattr_state enum is used to track various operations that are in
> progress so that we know not to repeat them, and resume where we left
> off before EAGAIN was returned to cycle out the transaction. Other
> members take the place of local variables that need to retain their
> values across multiple function recalls. See xfs_attr.h for a more
> detailed diagram of the states.
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
> fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
> fs/xfs/xfs_attr_inactive.c | 2 +-
> 6 files changed, 208 insertions(+), 66 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4b78c86..5c460f4 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> */
> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> struct xfs_da_state **state);
> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
> }
>
> /*
> + * Checks to see if a delayed attribute transaction should be rolled. If so,
> + * also checks for a defer finish. Transaction is finished and rolled as
> + * needed, and returns true of false if the delayed operation should continue.
> + */
> +bool
> +xfs_attr_roll_again(
The function name suggests this is more of a status checking function
than one that does actual work. I'd suggest something more like
xfs_attr_trans_roll() based on the implementation.
> + struct xfs_delattr_context *dac,
> + int *error)
> +{
> + struct xfs_da_args *args = dac->da_args;
> +
> + if (*error != -EAGAIN)
> + return false;
> +
> + if (dac->flags & XFS_DAC_DEFER_FINISH) {
> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
> + *error = xfs_defer_finish(&args->trans);
> + if (*error)
> + return false;
> + }
> +
I also find the semantics of this function a little confusing. How would
a caller distinguish between error == -EAGAIN as passed in from the
caller vs. error being set by one of the transaction processing calls?
> + *error = xfs_trans_roll_inode(&args->trans, args->dp);
> + return *error == 0;
> +}
> +
> +/*
> * Set the attribute specified in @args.
> */
> int
> @@ -364,23 +390,47 @@ xfs_has_attr(
> */
> int
> xfs_attr_remove_args(
> - struct xfs_da_args *args)
> + struct xfs_da_args *args)
> {
> - struct xfs_inode *dp = args->dp;
> - int error;
> + int error = 0;
> + struct xfs_delattr_context dac = {
> + .da_args = args,
> + };
> +
> + do {
> + error = xfs_attr_remove_iter(&dac);
> + } while (xfs_attr_roll_again(&dac, &error));
> +
> + return error;
> +}
> +
> +/*
> + * Remove the attribute specified in @args.
> + *
> + * This function may return -EAGAIN to signal that the transaction needs to be
> + * rolled. Callers should continue calling this function until they receive a
> + * return value other than -EAGAIN.
> + */
> +int
> +xfs_attr_remove_iter(
> + struct xfs_delattr_context *dac)
> +{
> + struct xfs_da_args *args = dac->da_args;
> + struct xfs_inode *dp = args->dp;
> +
> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> + goto node;
Thoughts on my comments on this on the previous version?
>
> if (!xfs_inode_hasattr(dp)) {
> - error = -ENOATTR;
> + return -ENOATTR;
> } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> - error = xfs_attr_shortform_remove(args);
> + return xfs_attr_shortform_remove(args);
> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> - error = xfs_attr_leaf_removename(args);
> - } else {
> - error = xfs_attr_node_removename(args);
> + return xfs_attr_leaf_removename(args);
> }
> -
> - return error;
> +node:
> + return xfs_attr_node_removename(dac);
> }
>
> /*
...
> @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
> error = xfs_da3_join(state);
> if (error)
> goto out;
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - goto out;
> - /*
> - * Commit the Btree join operation and start a new trans.
> - */
> - error = xfs_trans_roll_inode(&args->trans, dp);
> - if (error)
> - goto out;
> +
> + dac->flags |= XFS_DAC_DEFER_FINISH;
> + dac->dela_state = XFS_DAS_RM_SHRINK;
> + return -EAGAIN;
> }
>
> +das_rm_shrink:
> +
I think I also mentioned the tail of this function might sit better in
the caller..
Brian
> /*
> * If the result is small enough, push it all into the inode.
> */
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 3e97a93..6c58792 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
> };
>
>
> +/*
> + * ========================================================================
> + * Structure used to pass context around among the delayed routines.
> + * ========================================================================
> + */
> +
> +/*
> + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
> + * states indicate places where the function would return -EAGAIN, and then
> + * immediately resume from after being recalled by the calling function. States
> + * marked as a "subroutine state" indicate that they belong to a subroutine, and
> + * so the calling function needs to pass them back to that subroutine to allow
> + * it to finish where it left off. But they otherwise do not have a role in the
> + * calling function other than just passing through.
> + *
> + * xfs_attr_remove_iter()
> + * XFS_DAS_RM_SHRINK ─┐
> + * (subroutine state) │
> + * └─>xfs_attr_node_removename()
> + * │
> + * v
> + * need to
> + * shrink tree? ─n─┐
> + * │ │
> + * y │
> + * │ │
> + * v │
> + * XFS_DAS_RM_SHRINK │
> + * │ │
> + * v │
> + * done <─────┘
> + *
> + */
> +
> +/*
> + * Enum values for xfs_delattr_context.da_state
> + *
> + * These values are used by delayed attribute operations to keep track of where
> + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
> + * calling function to roll the transaction, and then recall the subroutine to
> + * finish the operation. The enum is then used by the subroutine to jump back
> + * to where it was and resume executing where it left off.
> + */
> +enum xfs_delattr_state {
> + /* Zero is uninitalized */
> + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
> +};
> +
> +/*
> + * Defines for xfs_delattr_context.flags
> + */
> +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
> +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
> +
> +/*
> + * Context used for keeping track of delayed attribute operations
> + */
> +struct xfs_delattr_context {
> + struct xfs_da_args *da_args;
> +
> + /* Used in xfs_attr_node_removename to roll through removing blocks */
> + struct xfs_da_state *da_state;
> + struct xfs_da_state_blk *blk;
> +
> + /* Used to keep track of current state of delayed operation */
> + unsigned int flags;
> + enum xfs_delattr_state dela_state;
> +};
> +
> /*========================================================================
> * Function prototypes for the kernel.
> *========================================================================*/
> @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
> int xfs_attr_set_args(struct xfs_da_args *args);
> int xfs_has_attr(struct xfs_da_args *args);
> int xfs_attr_remove_args(struct xfs_da_args *args);
> +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
> +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
> bool xfs_attr_namecheck(const void *name, size_t length);
> +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> + struct xfs_da_args *args);
>
> #endif /* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 351351c..20521bf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -19,8 +19,8 @@
> #include "xfs_bmap_btree.h"
> #include "xfs_bmap.h"
> #include "xfs_attr_sf.h"
> -#include "xfs_attr_remote.h"
> #include "xfs_attr.h"
> +#include "xfs_attr_remote.h"
> #include "xfs_attr_leaf.h"
> #include "xfs_error.h"
> #include "xfs_trace.h"
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 85dca51..20e4605 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
> */
> int
> xfs_attr_rmtval_remove(
> - struct xfs_da_args *args)
> + struct xfs_da_args *args)
> {
> - xfs_dablk_t lblkno;
> - int blkcnt;
> - int error = 0;
> - int retval = 0;
> + xfs_dablk_t lblkno;
> + int blkcnt;
> + int error = 0;
> + struct xfs_delattr_context dac = {
> + .da_args = args,
> + };
>
> trace_xfs_attr_rmtval_remove(args);
>
> @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
> lblkno = args->rmtblkno;
> blkcnt = args->rmtblkcnt;
> do {
> - retval = __xfs_attr_rmtval_remove(args);
> - if (retval && retval != EAGAIN)
> - return retval;
> -
> - /*
> - * Close out trans and start the next one in the chain.
> - */
> - error = xfs_trans_roll_inode(&args->trans, args->dp);
> - if (error)
> - return error;
> - } while (retval == -EAGAIN);
> + error = __xfs_attr_rmtval_remove(&dac);
> + } while (xfs_attr_roll_again(&dac, &error));
>
> - return 0;
> + return error;
> }
>
> /*
> @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
> */
> int
> __xfs_attr_rmtval_remove(
> - struct xfs_da_args *args)
> + struct xfs_delattr_context *dac)
> {
> - int error, done;
> + struct xfs_da_args *args = dac->da_args;
> + int error, done;
>
> /*
> * Unmap value blocks for this attr.
> @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
> if (error)
> return error;
>
> - error = xfs_defer_finish(&args->trans);
> - if (error)
> - return error;
> -
> - if (!done)
> + if (!done) {
> + dac->flags |= XFS_DAC_DEFER_FINISH;
> return -EAGAIN;
> + }
>
> return error;
> }
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> index 9eee615..002fd30 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.h
> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> @@ -14,5 +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_remove(struct xfs_da_args *args);
> +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
> #endif /* __XFS_ATTR_REMOTE_H__ */
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index bfad669..aaa7e66 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -15,10 +15,10 @@
> #include "xfs_da_format.h"
> #include "xfs_da_btree.h"
> #include "xfs_inode.h"
> +#include "xfs_attr.h"
> #include "xfs_attr_remote.h"
> #include "xfs_trans.h"
> #include "xfs_bmap.h"
> -#include "xfs_attr.h"
> #include "xfs_attr_leaf.h"
> #include "xfs_quota.h"
> #include "xfs_dir2.h"
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-09 13:24 ` Brian Foster
@ 2020-07-09 18:04 ` Darrick J. Wong
2020-07-10 6:22 ` Allison Collins
2020-07-13 13:36 ` Brian Foster
2020-07-09 22:02 ` Allison Collins
1 sibling, 2 replies; 37+ messages in thread
From: Darrick J. Wong @ 2020-07-09 18:04 UTC (permalink / raw)
To: Brian Foster; +Cc: Allison Collins, linux-xfs
On Thu, Jul 09, 2020 at 09:24:56AM -0400, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
> > This patch modifies the attr remove routines to be delay ready. This
> > means they no longer roll or commit transactions, but instead return
> > -EAGAIN to have the calling routine roll and refresh the transaction. In
> > this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
> > uses a sort of state machine like switch to keep track of where it was
> > when EAGAIN was returned. xfs_attr_node_removename has also been
> > modified to use the switch, and a new version of xfs_attr_remove_args
> > consists of a simple loop to refresh the transaction until the operation
> > is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
> > transaction where ever the existing code used to.
> >
> > Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> > version __xfs_attr_rmtval_remove. We will rename
> > __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> > done.
> >
> > xfs_attr_rmtval_remove itself is still in use by the set routines (used
> > during a rename). For reasons of perserving existing function, we
> > modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> > set. Similar to how xfs_attr_remove_args does here. Once we transition
> > the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> > used and will be removed.
> >
> > This patch also adds a new struct xfs_delattr_context, which we will use
> > to keep track of the current state of an attribute operation. The new
> > xfs_delattr_state enum is used to track various operations that are in
> > progress so that we know not to repeat them, and resume where we left
> > off before EAGAIN was returned to cycle out the transaction. Other
> > members take the place of local variables that need to retain their
> > values across multiple function recalls. See xfs_attr.h for a more
> > detailed diagram of the states.
> >
> > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
> > fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
> > fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> > fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
> > fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
> > fs/xfs/xfs_attr_inactive.c | 2 +-
> > 6 files changed, 208 insertions(+), 66 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 4b78c86..5c460f4 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> > */
> > STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> > STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> > -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> > +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
> > STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> > struct xfs_da_state **state);
> > STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> > @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
> > }
> >
> > /*
> > + * Checks to see if a delayed attribute transaction should be rolled. If so,
> > + * also checks for a defer finish. Transaction is finished and rolled as
> > + * needed, and returns true of false if the delayed operation should continue.
> > + */
> > +bool
> > +xfs_attr_roll_again(
>
> The function name suggests this is more of a status checking function
> than one that does actual work. I'd suggest something more like
> xfs_attr_trans_roll() based on the implementation.
>
> > + struct xfs_delattr_context *dac,
> > + int *error)
> > +{
> > + struct xfs_da_args *args = dac->da_args;
> > +
> > + if (*error != -EAGAIN)
> > + return false;
> > +
> > + if (dac->flags & XFS_DAC_DEFER_FINISH) {
> > + dac->flags &= ~XFS_DAC_DEFER_FINISH;
> > + *error = xfs_defer_finish(&args->trans);
> > + if (*error)
> > + return false;
> > + }
> > +
>
> I also find the semantics of this function a little confusing. How would
> a caller distinguish between error == -EAGAIN as passed in from the
> caller vs. error being set by one of the transaction processing calls?
Do any of them do that?
> > + *error = xfs_trans_roll_inode(&args->trans, args->dp);
I keep wondering why it's necessary to call xfs_defer_finish immediately
followed by xfs_trans_roll_inode. Maybe we've already resolved this
question, but if we did I don't remember the answer, and the answer
should be captured as a comment because I never remember. :P
Oh right, we /did/ resolve this question. The defer_finish exists to
finish all the deferred ops so that we return to the caller with a fresh
transaction and no dfops.
Comment, please...
if (dac->flags & XFS_DAC_DEFER_FINISH) {
/*
* The caller wants us to finish all the deferred ops so
* that we avoid pinning the log tail with a large
* number of deferred ops.
*/
dac->flags &= ~XFS_DAC_DEFER_FINISH;
*error = xfs_defer_finish(&args->trans);
if (*error)
return false;
}
Oh, another question: if we joined the inode to the transaction with
ilockflags == 0 (i.e. the transaction does not automatically unlock the
inode) then why do we need the separate _trans_roll_inode call? Won't
the _defer_finish call return with the inode joined to a clean
transaction?
(Or did we resolve this question already as well, and all we need is a
comment to remind your forgetful maintainer?)
--D
> > + return *error == 0;
> > +}
> > +
> > +/*
> > * Set the attribute specified in @args.
> > */
> > int
> > @@ -364,23 +390,47 @@ xfs_has_attr(
> > */
> > int
> > xfs_attr_remove_args(
> > - struct xfs_da_args *args)
> > + struct xfs_da_args *args)
> > {
> > - struct xfs_inode *dp = args->dp;
> > - int error;
> > + int error = 0;
> > + struct xfs_delattr_context dac = {
> > + .da_args = args,
> > + };
> > +
> > + do {
> > + error = xfs_attr_remove_iter(&dac);
> > + } while (xfs_attr_roll_again(&dac, &error));
> > +
> > + return error;
> > +}
> > +
> > +/*
> > + * Remove the attribute specified in @args.
> > + *
> > + * This function may return -EAGAIN to signal that the transaction needs to be
> > + * rolled. Callers should continue calling this function until they receive a
> > + * return value other than -EAGAIN.
> > + */
> > +int
> > +xfs_attr_remove_iter(
> > + struct xfs_delattr_context *dac)
> > +{
> > + struct xfs_da_args *args = dac->da_args;
> > + struct xfs_inode *dp = args->dp;
> > +
> > + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> > + goto node;
>
> Thoughts on my comments on this on the previous version?
>
> >
> > if (!xfs_inode_hasattr(dp)) {
> > - error = -ENOATTR;
> > + return -ENOATTR;
> > } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> > ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > - error = xfs_attr_shortform_remove(args);
> > + return xfs_attr_shortform_remove(args);
> > } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > - error = xfs_attr_leaf_removename(args);
> > - } else {
> > - error = xfs_attr_node_removename(args);
> > + return xfs_attr_leaf_removename(args);
> > }
> > -
> > - return error;
> > +node:
> > + return xfs_attr_node_removename(dac);
> > }
> >
> > /*
> ...
> > @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
> > error = xfs_da3_join(state);
> > if (error)
> > goto out;
> > - error = xfs_defer_finish(&args->trans);
> > - if (error)
> > - goto out;
> > - /*
> > - * Commit the Btree join operation and start a new trans.
> > - */
> > - error = xfs_trans_roll_inode(&args->trans, dp);
> > - if (error)
> > - goto out;
> > +
> > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > + dac->dela_state = XFS_DAS_RM_SHRINK;
> > + return -EAGAIN;
> > }
> >
> > +das_rm_shrink:
> > +
>
> I think I also mentioned the tail of this function might sit better in
> the caller..
>
> Brian
>
> > /*
> > * If the result is small enough, push it all into the inode.
> > */
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 3e97a93..6c58792 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
> > };
> >
> >
> > +/*
> > + * ========================================================================
> > + * Structure used to pass context around among the delayed routines.
> > + * ========================================================================
> > + */
> > +
> > +/*
> > + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
> > + * states indicate places where the function would return -EAGAIN, and then
> > + * immediately resume from after being recalled by the calling function. States
> > + * marked as a "subroutine state" indicate that they belong to a subroutine, and
> > + * so the calling function needs to pass them back to that subroutine to allow
> > + * it to finish where it left off. But they otherwise do not have a role in the
> > + * calling function other than just passing through.
> > + *
> > + * xfs_attr_remove_iter()
> > + * XFS_DAS_RM_SHRINK ─┐
> > + * (subroutine state) │
> > + * └─>xfs_attr_node_removename()
> > + * │
> > + * v
> > + * need to
> > + * shrink tree? ─n─┐
> > + * │ │
> > + * y │
> > + * │ │
> > + * v │
> > + * XFS_DAS_RM_SHRINK │
> > + * │ │
> > + * v │
> > + * done <─────┘
> > + *
> > + */
> > +
> > +/*
> > + * Enum values for xfs_delattr_context.da_state
> > + *
> > + * These values are used by delayed attribute operations to keep track of where
> > + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
> > + * calling function to roll the transaction, and then recall the subroutine to
> > + * finish the operation. The enum is then used by the subroutine to jump back
> > + * to where it was and resume executing where it left off.
> > + */
> > +enum xfs_delattr_state {
> > + /* Zero is uninitalized */
> > + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
> > +};
> > +
> > +/*
> > + * Defines for xfs_delattr_context.flags
> > + */
> > +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
> > +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
> > +
> > +/*
> > + * Context used for keeping track of delayed attribute operations
> > + */
> > +struct xfs_delattr_context {
> > + struct xfs_da_args *da_args;
> > +
> > + /* Used in xfs_attr_node_removename to roll through removing blocks */
> > + struct xfs_da_state *da_state;
> > + struct xfs_da_state_blk *blk;
> > +
> > + /* Used to keep track of current state of delayed operation */
> > + unsigned int flags;
> > + enum xfs_delattr_state dela_state;
> > +};
> > +
> > /*========================================================================
> > * Function prototypes for the kernel.
> > *========================================================================*/
> > @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
> > int xfs_attr_set_args(struct xfs_da_args *args);
> > int xfs_has_attr(struct xfs_da_args *args);
> > int xfs_attr_remove_args(struct xfs_da_args *args);
> > +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
> > +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
> > bool xfs_attr_namecheck(const void *name, size_t length);
> > +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> > + struct xfs_da_args *args);
> >
> > #endif /* __XFS_ATTR_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 351351c..20521bf 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -19,8 +19,8 @@
> > #include "xfs_bmap_btree.h"
> > #include "xfs_bmap.h"
> > #include "xfs_attr_sf.h"
> > -#include "xfs_attr_remote.h"
> > #include "xfs_attr.h"
> > +#include "xfs_attr_remote.h"
> > #include "xfs_attr_leaf.h"
> > #include "xfs_error.h"
> > #include "xfs_trace.h"
> > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > index 85dca51..20e4605 100644
> > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
> > */
> > int
> > xfs_attr_rmtval_remove(
> > - struct xfs_da_args *args)
> > + struct xfs_da_args *args)
> > {
> > - xfs_dablk_t lblkno;
> > - int blkcnt;
> > - int error = 0;
> > - int retval = 0;
> > + xfs_dablk_t lblkno;
> > + int blkcnt;
> > + int error = 0;
> > + struct xfs_delattr_context dac = {
> > + .da_args = args,
> > + };
> >
> > trace_xfs_attr_rmtval_remove(args);
> >
> > @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
> > lblkno = args->rmtblkno;
> > blkcnt = args->rmtblkcnt;
> > do {
> > - retval = __xfs_attr_rmtval_remove(args);
> > - if (retval && retval != EAGAIN)
> > - return retval;
> > -
> > - /*
> > - * Close out trans and start the next one in the chain.
> > - */
> > - error = xfs_trans_roll_inode(&args->trans, args->dp);
> > - if (error)
> > - return error;
> > - } while (retval == -EAGAIN);
> > + error = __xfs_attr_rmtval_remove(&dac);
> > + } while (xfs_attr_roll_again(&dac, &error));
> >
> > - return 0;
> > + return error;
> > }
> >
> > /*
> > @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
> > */
> > int
> > __xfs_attr_rmtval_remove(
> > - struct xfs_da_args *args)
> > + struct xfs_delattr_context *dac)
> > {
> > - int error, done;
> > + struct xfs_da_args *args = dac->da_args;
> > + int error, done;
> >
> > /*
> > * Unmap value blocks for this attr.
> > @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
> > if (error)
> > return error;
> >
> > - error = xfs_defer_finish(&args->trans);
> > - if (error)
> > - return error;
> > -
> > - if (!done)
> > + if (!done) {
> > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > return -EAGAIN;
> > + }
> >
> > return error;
> > }
> > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > index 9eee615..002fd30 100644
> > --- a/fs/xfs/libxfs/xfs_attr_remote.h
> > +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> > @@ -14,5 +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_remove(struct xfs_da_args *args);
> > +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
> > #endif /* __XFS_ATTR_REMOTE_H__ */
> > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > index bfad669..aaa7e66 100644
> > --- a/fs/xfs/xfs_attr_inactive.c
> > +++ b/fs/xfs/xfs_attr_inactive.c
> > @@ -15,10 +15,10 @@
> > #include "xfs_da_format.h"
> > #include "xfs_da_btree.h"
> > #include "xfs_inode.h"
> > +#include "xfs_attr.h"
> > #include "xfs_attr_remote.h"
> > #include "xfs_trans.h"
> > #include "xfs_bmap.h"
> > -#include "xfs_attr.h"
> > #include "xfs_attr_leaf.h"
> > #include "xfs_quota.h"
> > #include "xfs_dir2.h"
> > --
> > 2.7.4
> >
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-09 18:04 ` Darrick J. Wong
@ 2020-07-10 6:22 ` Allison Collins
2020-07-13 13:36 ` Brian Foster
1 sibling, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-07-10 6:22 UTC (permalink / raw)
To: Darrick J. Wong, Brian Foster; +Cc: linux-xfs
On 7/9/20 11:04 AM, Darrick J. Wong wrote:
> On Thu, Jul 09, 2020 at 09:24:56AM -0400, Brian Foster wrote:
>> On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
>>> This patch modifies the attr remove routines to be delay ready. This
>>> means they no longer roll or commit transactions, but instead return
>>> -EAGAIN to have the calling routine roll and refresh the transaction. In
>>> this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
>>> uses a sort of state machine like switch to keep track of where it was
>>> when EAGAIN was returned. xfs_attr_node_removename has also been
>>> modified to use the switch, and a new version of xfs_attr_remove_args
>>> consists of a simple loop to refresh the transaction until the operation
>>> is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
>>> transaction where ever the existing code used to.
>>>
>>> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
>>> version __xfs_attr_rmtval_remove. We will rename
>>> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
>>> done.
>>>
>>> xfs_attr_rmtval_remove itself is still in use by the set routines (used
>>> during a rename). For reasons of perserving existing function, we
>>> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
>>> set. Similar to how xfs_attr_remove_args does here. Once we transition
>>> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
>>> used and will be removed.
>>>
>>> This patch also adds a new struct xfs_delattr_context, which we will use
>>> to keep track of the current state of an attribute operation. The new
>>> xfs_delattr_state enum is used to track various operations that are in
>>> progress so that we know not to repeat them, and resume where we left
>>> off before EAGAIN was returned to cycle out the transaction. Other
>>> members take the place of local variables that need to retain their
>>> values across multiple function recalls. See xfs_attr.h for a more
>>> detailed diagram of the states.
>>>
>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>> ---
>>> fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
>>> fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
>>> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
>>> fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
>>> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
>>> fs/xfs/xfs_attr_inactive.c | 2 +-
>>> 6 files changed, 208 insertions(+), 66 deletions(-)
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>> index 4b78c86..5c460f4 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>> @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>> */
>>> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>> STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>> -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>>> +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
>>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>> struct xfs_da_state **state);
>>> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>>> @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
>>> }
>>>
>>> /*
>>> + * Checks to see if a delayed attribute transaction should be rolled. If so,
>>> + * also checks for a defer finish. Transaction is finished and rolled as
>>> + * needed, and returns true of false if the delayed operation should continue.
>>> + */
>>> +bool
>>> +xfs_attr_roll_again(
>>
>> The function name suggests this is more of a status checking function
>> than one that does actual work. I'd suggest something more like
>> xfs_attr_trans_roll() based on the implementation.
>>
>>> + struct xfs_delattr_context *dac,
>>> + int *error)
>>> +{
>>> + struct xfs_da_args *args = dac->da_args;
>>> +
>>> + if (*error != -EAGAIN)
>>> + return false;
>>> +
>>> + if (dac->flags & XFS_DAC_DEFER_FINISH) {
>>> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
>>> + *error = xfs_defer_finish(&args->trans);
>>> + if (*error)
>>> + return false;
>>> + }
>>> +
>>
>> I also find the semantics of this function a little confusing. How would
>> a caller distinguish between error == -EAGAIN as passed in from the
>> caller vs. error being set by one of the transaction processing calls?
>
> Do any of them do that?
>
>>> + *error = xfs_trans_roll_inode(&args->trans, args->dp);
>
> I keep wondering why it's necessary to call xfs_defer_finish immediately
> followed by xfs_trans_roll_inode. Maybe we've already resolved this
> question, but if we did I don't remember the answer, and the answer
> should be captured as a comment because I never remember. :P
>
> Oh right, we /did/ resolve this question. The defer_finish exists to
> finish all the deferred ops so that we return to the caller with a fresh
> transaction and no dfops.
>
> Comment, please...
>
> if (dac->flags & XFS_DAC_DEFER_FINISH) {
> /*
> * The caller wants us to finish all the deferred ops so
> * that we avoid pinning the log tail with a large
> * number of deferred ops.
> */
Sure, will add.
> dac->flags &= ~XFS_DAC_DEFER_FINISH;
> *error = xfs_defer_finish(&args->trans);
> if (*error)
> return false;
> }
>
> Oh, another question: if we joined the inode to the transaction with
> ilockflags == 0 (i.e. the transaction does not automatically unlock the
> inode) then why do we need the separate _trans_roll_inode call? Won't
> the _defer_finish call return with the inode joined to a clean
> transaction?
>
> (Or did we resolve this question already as well, and all we need is a
> comment to remind your forgetful maintainer?)
Mostly I think it was just because people wanted this to be a straight
hoist as much as possible. Reason being that the calling functions roll
the transaction whether or not the helpers call defer finish, so this
would be about equivalent.
In the case of the remove path, we probably could get away with skipping
the roll if we call defer finish, but then later when we call this from
the set routines, I think there's a race with the AIL such that we need
to roll the transaction and then join the leaf to the new transaction.
So then you'd need to hoist the roll back out, or plumb a flag in to
roll anyway, but I think that would make the semantics even more goofy.
I sort of get the impression that trying to stick to a "refactor only"
goal helps people to be a little more comfortable with the changes :-)
Allison
>
> --D
>
>>> + return *error == 0;
>>> +}
>>> +
>>> +/*
>>> * Set the attribute specified in @args.
>>> */
>>> int
>>> @@ -364,23 +390,47 @@ xfs_has_attr(
>>> */
>>> int
>>> xfs_attr_remove_args(
>>> - struct xfs_da_args *args)
>>> + struct xfs_da_args *args)
>>> {
>>> - struct xfs_inode *dp = args->dp;
>>> - int error;
>>> + int error = 0;
>>> + struct xfs_delattr_context dac = {
>>> + .da_args = args,
>>> + };
>>> +
>>> + do {
>>> + error = xfs_attr_remove_iter(&dac);
>>> + } while (xfs_attr_roll_again(&dac, &error));
>>> +
>>> + return error;
>>> +}
>>> +
>>> +/*
>>> + * Remove the attribute specified in @args.
>>> + *
>>> + * This function may return -EAGAIN to signal that the transaction needs to be
>>> + * rolled. Callers should continue calling this function until they receive a
>>> + * return value other than -EAGAIN.
>>> + */
>>> +int
>>> +xfs_attr_remove_iter(
>>> + struct xfs_delattr_context *dac)
>>> +{
>>> + struct xfs_da_args *args = dac->da_args;
>>> + struct xfs_inode *dp = args->dp;
>>> +
>>> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
>>> + goto node;
>>
>> Thoughts on my comments on this on the previous version?
>>
>>>
>>> if (!xfs_inode_hasattr(dp)) {
>>> - error = -ENOATTR;
>>> + return -ENOATTR;
>>> } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>>> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>>> - error = xfs_attr_shortform_remove(args);
>>> + return xfs_attr_shortform_remove(args);
>>> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>> - error = xfs_attr_leaf_removename(args);
>>> - } else {
>>> - error = xfs_attr_node_removename(args);
>>> + return xfs_attr_leaf_removename(args);
>>> }
>>> -
>>> - return error;
>>> +node:
>>> + return xfs_attr_node_removename(dac);
>>> }
>>>
>>> /*
>> ...
>>> @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
>>> error = xfs_da3_join(state);
>>> if (error)
>>> goto out;
>>> - error = xfs_defer_finish(&args->trans);
>>> - if (error)
>>> - goto out;
>>> - /*
>>> - * Commit the Btree join operation and start a new trans.
>>> - */
>>> - error = xfs_trans_roll_inode(&args->trans, dp);
>>> - if (error)
>>> - goto out;
>>> +
>>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>>> + dac->dela_state = XFS_DAS_RM_SHRINK;
>>> + return -EAGAIN;
>>> }
>>>
>>> +das_rm_shrink:
>>> +
>>
>> I think I also mentioned the tail of this function might sit better in
>> the caller..
>>
>> Brian
>>
>>> /*
>>> * If the result is small enough, push it all into the inode.
>>> */
>>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>>> index 3e97a93..6c58792 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.h
>>> +++ b/fs/xfs/libxfs/xfs_attr.h
>>> @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
>>> };
>>>
>>>
>>> +/*
>>> + * ========================================================================
>>> + * Structure used to pass context around among the delayed routines.
>>> + * ========================================================================
>>> + */
>>> +
>>> +/*
>>> + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
>>> + * states indicate places where the function would return -EAGAIN, and then
>>> + * immediately resume from after being recalled by the calling function. States
>>> + * marked as a "subroutine state" indicate that they belong to a subroutine, and
>>> + * so the calling function needs to pass them back to that subroutine to allow
>>> + * it to finish where it left off. But they otherwise do not have a role in the
>>> + * calling function other than just passing through.
>>> + *
>>> + * xfs_attr_remove_iter()
>>> + * XFS_DAS_RM_SHRINK ─┐
>>> + * (subroutine state) │
>>> + * └─>xfs_attr_node_removename()
>>> + * │
>>> + * v
>>> + * need to
>>> + * shrink tree? ─n─┐
>>> + * │ │
>>> + * y │
>>> + * │ │
>>> + * v │
>>> + * XFS_DAS_RM_SHRINK │
>>> + * │ │
>>> + * v │
>>> + * done <─────┘
>>> + *
>>> + */
>>> +
>>> +/*
>>> + * Enum values for xfs_delattr_context.da_state
>>> + *
>>> + * These values are used by delayed attribute operations to keep track of where
>>> + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
>>> + * calling function to roll the transaction, and then recall the subroutine to
>>> + * finish the operation. The enum is then used by the subroutine to jump back
>>> + * to where it was and resume executing where it left off.
>>> + */
>>> +enum xfs_delattr_state {
>>> + /* Zero is uninitalized */
>>> + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
>>> +};
>>> +
>>> +/*
>>> + * Defines for xfs_delattr_context.flags
>>> + */
>>> +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
>>> +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
>>> +
>>> +/*
>>> + * Context used for keeping track of delayed attribute operations
>>> + */
>>> +struct xfs_delattr_context {
>>> + struct xfs_da_args *da_args;
>>> +
>>> + /* Used in xfs_attr_node_removename to roll through removing blocks */
>>> + struct xfs_da_state *da_state;
>>> + struct xfs_da_state_blk *blk;
>>> +
>>> + /* Used to keep track of current state of delayed operation */
>>> + unsigned int flags;
>>> + enum xfs_delattr_state dela_state;
>>> +};
>>> +
>>> /*========================================================================
>>> * Function prototypes for the kernel.
>>> *========================================================================*/
>>> @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
>>> int xfs_attr_set_args(struct xfs_da_args *args);
>>> int xfs_has_attr(struct xfs_da_args *args);
>>> int xfs_attr_remove_args(struct xfs_da_args *args);
>>> +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
>>> +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
>>> bool xfs_attr_namecheck(const void *name, size_t length);
>>> +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
>>> + struct xfs_da_args *args);
>>>
>>> #endif /* __XFS_ATTR_H__ */
>>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>>> index 351351c..20521bf 100644
>>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>>> @@ -19,8 +19,8 @@
>>> #include "xfs_bmap_btree.h"
>>> #include "xfs_bmap.h"
>>> #include "xfs_attr_sf.h"
>>> -#include "xfs_attr_remote.h"
>>> #include "xfs_attr.h"
>>> +#include "xfs_attr_remote.h"
>>> #include "xfs_attr_leaf.h"
>>> #include "xfs_error.h"
>>> #include "xfs_trace.h"
>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>>> index 85dca51..20e4605 100644
>>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>>> @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
>>> */
>>> int
>>> xfs_attr_rmtval_remove(
>>> - struct xfs_da_args *args)
>>> + struct xfs_da_args *args)
>>> {
>>> - xfs_dablk_t lblkno;
>>> - int blkcnt;
>>> - int error = 0;
>>> - int retval = 0;
>>> + xfs_dablk_t lblkno;
>>> + int blkcnt;
>>> + int error = 0;
>>> + struct xfs_delattr_context dac = {
>>> + .da_args = args,
>>> + };
>>>
>>> trace_xfs_attr_rmtval_remove(args);
>>>
>>> @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
>>> lblkno = args->rmtblkno;
>>> blkcnt = args->rmtblkcnt;
>>> do {
>>> - retval = __xfs_attr_rmtval_remove(args);
>>> - if (retval && retval != EAGAIN)
>>> - return retval;
>>> -
>>> - /*
>>> - * Close out trans and start the next one in the chain.
>>> - */
>>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>>> - if (error)
>>> - return error;
>>> - } while (retval == -EAGAIN);
>>> + error = __xfs_attr_rmtval_remove(&dac);
>>> + } while (xfs_attr_roll_again(&dac, &error));
>>>
>>> - return 0;
>>> + return error;
>>> }
>>>
>>> /*
>>> @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
>>> */
>>> int
>>> __xfs_attr_rmtval_remove(
>>> - struct xfs_da_args *args)
>>> + struct xfs_delattr_context *dac)
>>> {
>>> - int error, done;
>>> + struct xfs_da_args *args = dac->da_args;
>>> + int error, done;
>>>
>>> /*
>>> * Unmap value blocks for this attr.
>>> @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
>>> if (error)
>>> return error;
>>>
>>> - error = xfs_defer_finish(&args->trans);
>>> - if (error)
>>> - return error;
>>> -
>>> - if (!done)
>>> + if (!done) {
>>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>>> return -EAGAIN;
>>> + }
>>>
>>> return error;
>>> }
>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>>> index 9eee615..002fd30 100644
>>> --- a/fs/xfs/libxfs/xfs_attr_remote.h
>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
>>> @@ -14,5 +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_remove(struct xfs_da_args *args);
>>> +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
>>> #endif /* __XFS_ATTR_REMOTE_H__ */
>>> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
>>> index bfad669..aaa7e66 100644
>>> --- a/fs/xfs/xfs_attr_inactive.c
>>> +++ b/fs/xfs/xfs_attr_inactive.c
>>> @@ -15,10 +15,10 @@
>>> #include "xfs_da_format.h"
>>> #include "xfs_da_btree.h"
>>> #include "xfs_inode.h"
>>> +#include "xfs_attr.h"
>>> #include "xfs_attr_remote.h"
>>> #include "xfs_trans.h"
>>> #include "xfs_bmap.h"
>>> -#include "xfs_attr.h"
>>> #include "xfs_attr_leaf.h"
>>> #include "xfs_quota.h"
>>> #include "xfs_dir2.h"
>>> --
>>> 2.7.4
>>>
>>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-09 18:04 ` Darrick J. Wong
2020-07-10 6:22 ` Allison Collins
@ 2020-07-13 13:36 ` Brian Foster
1 sibling, 0 replies; 37+ messages in thread
From: Brian Foster @ 2020-07-13 13:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Allison Collins, linux-xfs
On Thu, Jul 09, 2020 at 11:04:02AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 09, 2020 at 09:24:56AM -0400, Brian Foster wrote:
> > On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
> > > This patch modifies the attr remove routines to be delay ready. This
> > > means they no longer roll or commit transactions, but instead return
> > > -EAGAIN to have the calling routine roll and refresh the transaction. In
> > > this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
> > > uses a sort of state machine like switch to keep track of where it was
> > > when EAGAIN was returned. xfs_attr_node_removename has also been
> > > modified to use the switch, and a new version of xfs_attr_remove_args
> > > consists of a simple loop to refresh the transaction until the operation
> > > is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
> > > transaction where ever the existing code used to.
> > >
> > > Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> > > version __xfs_attr_rmtval_remove. We will rename
> > > __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> > > done.
> > >
> > > xfs_attr_rmtval_remove itself is still in use by the set routines (used
> > > during a rename). For reasons of perserving existing function, we
> > > modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> > > set. Similar to how xfs_attr_remove_args does here. Once we transition
> > > the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> > > used and will be removed.
> > >
> > > This patch also adds a new struct xfs_delattr_context, which we will use
> > > to keep track of the current state of an attribute operation. The new
> > > xfs_delattr_state enum is used to track various operations that are in
> > > progress so that we know not to repeat them, and resume where we left
> > > off before EAGAIN was returned to cycle out the transaction. Other
> > > members take the place of local variables that need to retain their
> > > values across multiple function recalls. See xfs_attr.h for a more
> > > detailed diagram of the states.
> > >
> > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > ---
> > > fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
> > > fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
> > > fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> > > fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
> > > fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
> > > fs/xfs/xfs_attr_inactive.c | 2 +-
> > > 6 files changed, 208 insertions(+), 66 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 4b78c86..5c460f4 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> > > */
> > > STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> > > STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> > > -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> > > +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
> > > STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> > > struct xfs_da_state **state);
> > > STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> > > @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
> > > }
> > >
> > > /*
> > > + * Checks to see if a delayed attribute transaction should be rolled. If so,
> > > + * also checks for a defer finish. Transaction is finished and rolled as
> > > + * needed, and returns true of false if the delayed operation should continue.
> > > + */
> > > +bool
> > > +xfs_attr_roll_again(
> >
> > The function name suggests this is more of a status checking function
> > than one that does actual work. I'd suggest something more like
> > xfs_attr_trans_roll() based on the implementation.
> >
> > > + struct xfs_delattr_context *dac,
> > > + int *error)
> > > +{
> > > + struct xfs_da_args *args = dac->da_args;
> > > +
> > > + if (*error != -EAGAIN)
> > > + return false;
> > > +
> > > + if (dac->flags & XFS_DAC_DEFER_FINISH) {
> > > + dac->flags &= ~XFS_DAC_DEFER_FINISH;
> > > + *error = xfs_defer_finish(&args->trans);
> > > + if (*error)
> > > + return false;
> > > + }
> > > +
> >
> > I also find the semantics of this function a little confusing. How would
> > a caller distinguish between error == -EAGAIN as passed in from the
> > caller vs. error being set by one of the transaction processing calls?
>
> Do any of them do that?
>
Not sure, but I doubt it. I didn't really look because the nature of the
feedback wasn't necessarily to suggest there was a bug, but rather that
the logic of the helper seems overloaded/convoluted. This has two
callers so I don't see why we wouldn't let the caller context determine
when to roll (i.e. error == -EAGAIN), implement the helper as a
deliberate transaction roll and make it return an error code that
properly flows back up the callchain if any part of the roll actually
fails.
> > > + *error = xfs_trans_roll_inode(&args->trans, args->dp);
>
> I keep wondering why it's necessary to call xfs_defer_finish immediately
> followed by xfs_trans_roll_inode. Maybe we've already resolved this
> question, but if we did I don't remember the answer, and the answer
> should be captured as a comment because I never remember. :P
>
> Oh right, we /did/ resolve this question. The defer_finish exists to
> finish all the deferred ops so that we return to the caller with a fresh
> transaction and no dfops.
>
There's also a subtle relationship that completing deferred operations
might be required for crash recovery integrity in certain cases. I've
just discovered a similar issue with my recent insert range changes that
broke things (patch coming later..) because rolling the transaction
alone defers the intents for the rmapbt changes that were associated
with the changes in the primary transaction rather than immediately
logging them. I suspect we might want to think about ways to improve
this interface because currently that is rather easy to miss.
Brian
> Comment, please...
>
> if (dac->flags & XFS_DAC_DEFER_FINISH) {
> /*
> * The caller wants us to finish all the deferred ops so
> * that we avoid pinning the log tail with a large
> * number of deferred ops.
> */
> dac->flags &= ~XFS_DAC_DEFER_FINISH;
> *error = xfs_defer_finish(&args->trans);
> if (*error)
> return false;
> }
>
> Oh, another question: if we joined the inode to the transaction with
> ilockflags == 0 (i.e. the transaction does not automatically unlock the
> inode) then why do we need the separate _trans_roll_inode call? Won't
> the _defer_finish call return with the inode joined to a clean
> transaction?
>
> (Or did we resolve this question already as well, and all we need is a
> comment to remind your forgetful maintainer?)
>
> --D
>
> > > + return *error == 0;
> > > +}
> > > +
> > > +/*
> > > * Set the attribute specified in @args.
> > > */
> > > int
> > > @@ -364,23 +390,47 @@ xfs_has_attr(
> > > */
> > > int
> > > xfs_attr_remove_args(
> > > - struct xfs_da_args *args)
> > > + struct xfs_da_args *args)
> > > {
> > > - struct xfs_inode *dp = args->dp;
> > > - int error;
> > > + int error = 0;
> > > + struct xfs_delattr_context dac = {
> > > + .da_args = args,
> > > + };
> > > +
> > > + do {
> > > + error = xfs_attr_remove_iter(&dac);
> > > + } while (xfs_attr_roll_again(&dac, &error));
> > > +
> > > + return error;
> > > +}
> > > +
> > > +/*
> > > + * Remove the attribute specified in @args.
> > > + *
> > > + * This function may return -EAGAIN to signal that the transaction needs to be
> > > + * rolled. Callers should continue calling this function until they receive a
> > > + * return value other than -EAGAIN.
> > > + */
> > > +int
> > > +xfs_attr_remove_iter(
> > > + struct xfs_delattr_context *dac)
> > > +{
> > > + struct xfs_da_args *args = dac->da_args;
> > > + struct xfs_inode *dp = args->dp;
> > > +
> > > + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> > > + goto node;
> >
> > Thoughts on my comments on this on the previous version?
> >
> > >
> > > if (!xfs_inode_hasattr(dp)) {
> > > - error = -ENOATTR;
> > > + return -ENOATTR;
> > > } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> > > ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > > - error = xfs_attr_shortform_remove(args);
> > > + return xfs_attr_shortform_remove(args);
> > > } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > > - error = xfs_attr_leaf_removename(args);
> > > - } else {
> > > - error = xfs_attr_node_removename(args);
> > > + return xfs_attr_leaf_removename(args);
> > > }
> > > -
> > > - return error;
> > > +node:
> > > + return xfs_attr_node_removename(dac);
> > > }
> > >
> > > /*
> > ...
> > > @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
> > > error = xfs_da3_join(state);
> > > if (error)
> > > goto out;
> > > - error = xfs_defer_finish(&args->trans);
> > > - if (error)
> > > - goto out;
> > > - /*
> > > - * Commit the Btree join operation and start a new trans.
> > > - */
> > > - error = xfs_trans_roll_inode(&args->trans, dp);
> > > - if (error)
> > > - goto out;
> > > +
> > > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > > + dac->dela_state = XFS_DAS_RM_SHRINK;
> > > + return -EAGAIN;
> > > }
> > >
> > > +das_rm_shrink:
> > > +
> >
> > I think I also mentioned the tail of this function might sit better in
> > the caller..
> >
> > Brian
> >
> > > /*
> > > * If the result is small enough, push it all into the inode.
> > > */
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index 3e97a93..6c58792 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
> > > };
> > >
> > >
> > > +/*
> > > + * ========================================================================
> > > + * Structure used to pass context around among the delayed routines.
> > > + * ========================================================================
> > > + */
> > > +
> > > +/*
> > > + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
> > > + * states indicate places where the function would return -EAGAIN, and then
> > > + * immediately resume from after being recalled by the calling function. States
> > > + * marked as a "subroutine state" indicate that they belong to a subroutine, and
> > > + * so the calling function needs to pass them back to that subroutine to allow
> > > + * it to finish where it left off. But they otherwise do not have a role in the
> > > + * calling function other than just passing through.
> > > + *
> > > + * xfs_attr_remove_iter()
> > > + * XFS_DAS_RM_SHRINK ─┐
> > > + * (subroutine state) │
> > > + * └─>xfs_attr_node_removename()
> > > + * │
> > > + * v
> > > + * need to
> > > + * shrink tree? ─n─┐
> > > + * │ │
> > > + * y │
> > > + * │ │
> > > + * v │
> > > + * XFS_DAS_RM_SHRINK │
> > > + * │ │
> > > + * v │
> > > + * done <─────┘
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Enum values for xfs_delattr_context.da_state
> > > + *
> > > + * These values are used by delayed attribute operations to keep track of where
> > > + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
> > > + * calling function to roll the transaction, and then recall the subroutine to
> > > + * finish the operation. The enum is then used by the subroutine to jump back
> > > + * to where it was and resume executing where it left off.
> > > + */
> > > +enum xfs_delattr_state {
> > > + /* Zero is uninitalized */
> > > + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
> > > +};
> > > +
> > > +/*
> > > + * Defines for xfs_delattr_context.flags
> > > + */
> > > +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
> > > +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
> > > +
> > > +/*
> > > + * Context used for keeping track of delayed attribute operations
> > > + */
> > > +struct xfs_delattr_context {
> > > + struct xfs_da_args *da_args;
> > > +
> > > + /* Used in xfs_attr_node_removename to roll through removing blocks */
> > > + struct xfs_da_state *da_state;
> > > + struct xfs_da_state_blk *blk;
> > > +
> > > + /* Used to keep track of current state of delayed operation */
> > > + unsigned int flags;
> > > + enum xfs_delattr_state dela_state;
> > > +};
> > > +
> > > /*========================================================================
> > > * Function prototypes for the kernel.
> > > *========================================================================*/
> > > @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
> > > int xfs_attr_set_args(struct xfs_da_args *args);
> > > int xfs_has_attr(struct xfs_da_args *args);
> > > int xfs_attr_remove_args(struct xfs_da_args *args);
> > > +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
> > > +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
> > > bool xfs_attr_namecheck(const void *name, size_t length);
> > > +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> > > + struct xfs_da_args *args);
> > >
> > > #endif /* __XFS_ATTR_H__ */
> > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > > index 351351c..20521bf 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > > @@ -19,8 +19,8 @@
> > > #include "xfs_bmap_btree.h"
> > > #include "xfs_bmap.h"
> > > #include "xfs_attr_sf.h"
> > > -#include "xfs_attr_remote.h"
> > > #include "xfs_attr.h"
> > > +#include "xfs_attr_remote.h"
> > > #include "xfs_attr_leaf.h"
> > > #include "xfs_error.h"
> > > #include "xfs_trace.h"
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > index 85dca51..20e4605 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
> > > */
> > > int
> > > xfs_attr_rmtval_remove(
> > > - struct xfs_da_args *args)
> > > + struct xfs_da_args *args)
> > > {
> > > - xfs_dablk_t lblkno;
> > > - int blkcnt;
> > > - int error = 0;
> > > - int retval = 0;
> > > + xfs_dablk_t lblkno;
> > > + int blkcnt;
> > > + int error = 0;
> > > + struct xfs_delattr_context dac = {
> > > + .da_args = args,
> > > + };
> > >
> > > trace_xfs_attr_rmtval_remove(args);
> > >
> > > @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
> > > lblkno = args->rmtblkno;
> > > blkcnt = args->rmtblkcnt;
> > > do {
> > > - retval = __xfs_attr_rmtval_remove(args);
> > > - if (retval && retval != EAGAIN)
> > > - return retval;
> > > -
> > > - /*
> > > - * Close out trans and start the next one in the chain.
> > > - */
> > > - error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > - if (error)
> > > - return error;
> > > - } while (retval == -EAGAIN);
> > > + error = __xfs_attr_rmtval_remove(&dac);
> > > + } while (xfs_attr_roll_again(&dac, &error));
> > >
> > > - return 0;
> > > + return error;
> > > }
> > >
> > > /*
> > > @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
> > > */
> > > int
> > > __xfs_attr_rmtval_remove(
> > > - struct xfs_da_args *args)
> > > + struct xfs_delattr_context *dac)
> > > {
> > > - int error, done;
> > > + struct xfs_da_args *args = dac->da_args;
> > > + int error, done;
> > >
> > > /*
> > > * Unmap value blocks for this attr.
> > > @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
> > > if (error)
> > > return error;
> > >
> > > - error = xfs_defer_finish(&args->trans);
> > > - if (error)
> > > - return error;
> > > -
> > > - if (!done)
> > > + if (!done) {
> > > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > > return -EAGAIN;
> > > + }
> > >
> > > return error;
> > > }
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > > index 9eee615..002fd30 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.h
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> > > @@ -14,5 +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_remove(struct xfs_da_args *args);
> > > +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
> > > #endif /* __XFS_ATTR_REMOTE_H__ */
> > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > > index bfad669..aaa7e66 100644
> > > --- a/fs/xfs/xfs_attr_inactive.c
> > > +++ b/fs/xfs/xfs_attr_inactive.c
> > > @@ -15,10 +15,10 @@
> > > #include "xfs_da_format.h"
> > > #include "xfs_da_btree.h"
> > > #include "xfs_inode.h"
> > > +#include "xfs_attr.h"
> > > #include "xfs_attr_remote.h"
> > > #include "xfs_trans.h"
> > > #include "xfs_bmap.h"
> > > -#include "xfs_attr.h"
> > > #include "xfs_attr_leaf.h"
> > > #include "xfs_quota.h"
> > > #include "xfs_dir2.h"
> > > --
> > > 2.7.4
> > >
> >
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-09 13:24 ` Brian Foster
2020-07-09 18:04 ` Darrick J. Wong
@ 2020-07-09 22:02 ` Allison Collins
2020-07-13 13:58 ` Brian Foster
1 sibling, 1 reply; 37+ messages in thread
From: Allison Collins @ 2020-07-09 22:02 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 7/9/20 6:24 AM, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
>> This patch modifies the attr remove routines to be delay ready. This
>> means they no longer roll or commit transactions, but instead return
>> -EAGAIN to have the calling routine roll and refresh the transaction. In
>> this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
>> uses a sort of state machine like switch to keep track of where it was
>> when EAGAIN was returned. xfs_attr_node_removename has also been
>> modified to use the switch, and a new version of xfs_attr_remove_args
>> consists of a simple loop to refresh the transaction until the operation
>> is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
>> transaction where ever the existing code used to.
>>
>> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
>> version __xfs_attr_rmtval_remove. We will rename
>> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
>> done.
>>
>> xfs_attr_rmtval_remove itself is still in use by the set routines (used
>> during a rename). For reasons of perserving existing function, we
>> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
>> set. Similar to how xfs_attr_remove_args does here. Once we transition
>> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
>> used and will be removed.
>>
>> This patch also adds a new struct xfs_delattr_context, which we will use
>> to keep track of the current state of an attribute operation. The new
>> xfs_delattr_state enum is used to track various operations that are in
>> progress so that we know not to repeat them, and resume where we left
>> off before EAGAIN was returned to cycle out the transaction. Other
>> members take the place of local variables that need to retain their
>> values across multiple function recalls. See xfs_attr.h for a more
>> detailed diagram of the states.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
>> fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
>> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
>> fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
>> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
>> fs/xfs/xfs_attr_inactive.c | 2 +-
>> 6 files changed, 208 insertions(+), 66 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4b78c86..5c460f4 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>> */
>> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>> STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>> -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>> +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>> struct xfs_da_state **state);
>> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>> @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
>> }
>>
>> /*
>> + * Checks to see if a delayed attribute transaction should be rolled. If so,
>> + * also checks for a defer finish. Transaction is finished and rolled as
>> + * needed, and returns true of false if the delayed operation should continue.
>> + */
>> +bool
>> +xfs_attr_roll_again(
>
> The function name suggests this is more of a status checking function
> than one that does actual work. I'd suggest something more like
> xfs_attr_trans_roll() based on the implementation.
Sure, I believe Darrick had asked for this helper in the last review, so
as long as folks are in agreement with the name, that is fine.
>
>> + struct xfs_delattr_context *dac,
>> + int *error)
>> +{
>> + struct xfs_da_args *args = dac->da_args;
>> +
>> + if (*error != -EAGAIN)
>> + return false;
>> +
>> + if (dac->flags & XFS_DAC_DEFER_FINISH) {
>> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
>> + *error = xfs_defer_finish(&args->trans);
>> + if (*error)
>> + return false;
>> + }
>> +
>
> I also find the semantics of this function a little confusing. How would
> a caller distinguish between error == -EAGAIN as passed in from the
> caller vs. error being set by one of the transaction processing calls?
I believe the helper was just meant to be a straight hoist from the v9
version. The error parameter here is an input for use in this scope,
it's not for the caller to do anything with. Other than to pipe it back
in. The caller just needs to know whether or not to continue.
That signature is a bit odd, though this pattern does appear in a few
places around the code, so it does help consolidate a bit. I suppose
it's just a question of which people prefer.
>
>> + *error = xfs_trans_roll_inode(&args->trans, args->dp);
>> + return *error == 0;
>> +}
>> +
>> +/*
>> * Set the attribute specified in @args.
>> */
>> int
>> @@ -364,23 +390,47 @@ xfs_has_attr(
>> */
>> int
>> xfs_attr_remove_args(
>> - struct xfs_da_args *args)
>> + struct xfs_da_args *args)
>> {
>> - struct xfs_inode *dp = args->dp;
>> - int error;
>> + int error = 0;
>> + struct xfs_delattr_context dac = {
>> + .da_args = args,
>> + };
>> +
>> + do {
>> + error = xfs_attr_remove_iter(&dac);
>> + } while (xfs_attr_roll_again(&dac, &error));
>> +
>> + return error;
>> +}
>> +
>> +/*
>> + * Remove the attribute specified in @args.
>> + *
>> + * This function may return -EAGAIN to signal that the transaction needs to be
>> + * rolled. Callers should continue calling this function until they receive a
>> + * return value other than -EAGAIN.
>> + */
>> +int
>> +xfs_attr_remove_iter(
>> + struct xfs_delattr_context *dac)
>> +{
>> + struct xfs_da_args *args = dac->da_args;
>> + struct xfs_inode *dp = args->dp;
>> +
>> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
>> + goto node;
>
> Thoughts on my comments on this on the previous version?
Apologies, I must have missed responding to it, I had a few family
emergencies at the time and must have lost track. I do believe we need
the goto because in the process of shrinking, it is possible that we
turn back into a block which would cause us to fall into the wrong
if/else logic with out it. I will run the test and double check though.
I'm pretty sure I recall running into that though.
>
>>
>> if (!xfs_inode_hasattr(dp)) {
>> - error = -ENOATTR;
>> + return -ENOATTR;
>> } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>> - error = xfs_attr_shortform_remove(args);
>> + return xfs_attr_shortform_remove(args);
>> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>> - error = xfs_attr_leaf_removename(args);
>> - } else {
>> - error = xfs_attr_node_removename(args);
>> + return xfs_attr_leaf_removename(args);
>> }
>> -
>> - return error;
>> +node:
>> + return xfs_attr_node_removename(dac);
>> }
>>
>> /*
> ...
>> @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
>> error = xfs_da3_join(state);
>> if (error)
>> goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> - /*
>> - * Commit the Btree join operation and start a new trans.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - goto out;
>> +
>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>> + dac->dela_state = XFS_DAS_RM_SHRINK;
>> + return -EAGAIN;
>> }
>>
>> +das_rm_shrink:
>> +
>
> I think I also mentioned the tail of this function might sit better in
> the caller..
I see, yes I'll see if I could pull that up. If I missed anything else,
please call it out. Thank you for your reviews!
Allison
>
> Brian
>
>> /*
>> * If the result is small enough, push it all into the inode.
>> */
>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>> index 3e97a93..6c58792 100644
>> --- a/fs/xfs/libxfs/xfs_attr.h
>> +++ b/fs/xfs/libxfs/xfs_attr.h
>> @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
>> };
>>
>>
>> +/*
>> + * ========================================================================
>> + * Structure used to pass context around among the delayed routines.
>> + * ========================================================================
>> + */
>> +
>> +/*
>> + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
>> + * states indicate places where the function would return -EAGAIN, and then
>> + * immediately resume from after being recalled by the calling function. States
>> + * marked as a "subroutine state" indicate that they belong to a subroutine, and
>> + * so the calling function needs to pass them back to that subroutine to allow
>> + * it to finish where it left off. But they otherwise do not have a role in the
>> + * calling function other than just passing through.
>> + *
>> + * xfs_attr_remove_iter()
>> + * XFS_DAS_RM_SHRINK ─┐
>> + * (subroutine state) │
>> + * └─>xfs_attr_node_removename()
>> + * │
>> + * v
>> + * need to
>> + * shrink tree? ─n─┐
>> + * │ │
>> + * y │
>> + * │ │
>> + * v │
>> + * XFS_DAS_RM_SHRINK │
>> + * │ │
>> + * v │
>> + * done <─────┘
>> + *
>> + */
>> +
>> +/*
>> + * Enum values for xfs_delattr_context.da_state
>> + *
>> + * These values are used by delayed attribute operations to keep track of where
>> + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
>> + * calling function to roll the transaction, and then recall the subroutine to
>> + * finish the operation. The enum is then used by the subroutine to jump back
>> + * to where it was and resume executing where it left off.
>> + */
>> +enum xfs_delattr_state {
>> + /* Zero is uninitalized */
>> + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
>> +};
>> +
>> +/*
>> + * Defines for xfs_delattr_context.flags
>> + */
>> +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
>> +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
>> +
>> +/*
>> + * Context used for keeping track of delayed attribute operations
>> + */
>> +struct xfs_delattr_context {
>> + struct xfs_da_args *da_args;
>> +
>> + /* Used in xfs_attr_node_removename to roll through removing blocks */
>> + struct xfs_da_state *da_state;
>> + struct xfs_da_state_blk *blk;
>> +
>> + /* Used to keep track of current state of delayed operation */
>> + unsigned int flags;
>> + enum xfs_delattr_state dela_state;
>> +};
>> +
>> /*========================================================================
>> * Function prototypes for the kernel.
>> *========================================================================*/
>> @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
>> int xfs_attr_set_args(struct xfs_da_args *args);
>> int xfs_has_attr(struct xfs_da_args *args);
>> int xfs_attr_remove_args(struct xfs_da_args *args);
>> +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
>> +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
>> bool xfs_attr_namecheck(const void *name, size_t length);
>> +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
>> + struct xfs_da_args *args);
>>
>> #endif /* __XFS_ATTR_H__ */
>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>> index 351351c..20521bf 100644
>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>> @@ -19,8 +19,8 @@
>> #include "xfs_bmap_btree.h"
>> #include "xfs_bmap.h"
>> #include "xfs_attr_sf.h"
>> -#include "xfs_attr_remote.h"
>> #include "xfs_attr.h"
>> +#include "xfs_attr_remote.h"
>> #include "xfs_attr_leaf.h"
>> #include "xfs_error.h"
>> #include "xfs_trace.h"
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index 85dca51..20e4605 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
>> */
>> int
>> xfs_attr_rmtval_remove(
>> - struct xfs_da_args *args)
>> + struct xfs_da_args *args)
>> {
>> - xfs_dablk_t lblkno;
>> - int blkcnt;
>> - int error = 0;
>> - int retval = 0;
>> + xfs_dablk_t lblkno;
>> + int blkcnt;
>> + int error = 0;
>> + struct xfs_delattr_context dac = {
>> + .da_args = args,
>> + };
>>
>> trace_xfs_attr_rmtval_remove(args);
>>
>> @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
>> lblkno = args->rmtblkno;
>> blkcnt = args->rmtblkcnt;
>> do {
>> - retval = __xfs_attr_rmtval_remove(args);
>> - if (retval && retval != EAGAIN)
>> - return retval;
>> -
>> - /*
>> - * Close out trans and start the next one in the chain.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> - if (error)
>> - return error;
>> - } while (retval == -EAGAIN);
>> + error = __xfs_attr_rmtval_remove(&dac);
>> + } while (xfs_attr_roll_again(&dac, &error));
>>
>> - return 0;
>> + return error;
>> }
>>
>> /*
>> @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
>> */
>> int
>> __xfs_attr_rmtval_remove(
>> - struct xfs_da_args *args)
>> + struct xfs_delattr_context *dac)
>> {
>> - int error, done;
>> + struct xfs_da_args *args = dac->da_args;
>> + int error, done;
>>
>> /*
>> * Unmap value blocks for this attr.
>> @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
>> if (error)
>> return error;
>>
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> -
>> - if (!done)
>> + if (!done) {
>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>> return -EAGAIN;
>> + }
>>
>> return error;
>> }
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>> index 9eee615..002fd30 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.h
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
>> @@ -14,5 +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_remove(struct xfs_da_args *args);
>> +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
>> #endif /* __XFS_ATTR_REMOTE_H__ */
>> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
>> index bfad669..aaa7e66 100644
>> --- a/fs/xfs/xfs_attr_inactive.c
>> +++ b/fs/xfs/xfs_attr_inactive.c
>> @@ -15,10 +15,10 @@
>> #include "xfs_da_format.h"
>> #include "xfs_da_btree.h"
>> #include "xfs_inode.h"
>> +#include "xfs_attr.h"
>> #include "xfs_attr_remote.h"
>> #include "xfs_trans.h"
>> #include "xfs_bmap.h"
>> -#include "xfs_attr.h"
>> #include "xfs_attr_leaf.h"
>> #include "xfs_quota.h"
>> #include "xfs_dir2.h"
>> --
>> 2.7.4
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-09 22:02 ` Allison Collins
@ 2020-07-13 13:58 ` Brian Foster
2020-07-15 5:04 ` Allison Collins
0 siblings, 1 reply; 37+ messages in thread
From: Brian Foster @ 2020-07-13 13:58 UTC (permalink / raw)
To: Allison Collins; +Cc: linux-xfs
On Thu, Jul 09, 2020 at 03:02:08PM -0700, Allison Collins wrote:
>
>
> On 7/9/20 6:24 AM, Brian Foster wrote:
> > On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
> > > This patch modifies the attr remove routines to be delay ready. This
> > > means they no longer roll or commit transactions, but instead return
> > > -EAGAIN to have the calling routine roll and refresh the transaction. In
> > > this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
> > > uses a sort of state machine like switch to keep track of where it was
> > > when EAGAIN was returned. xfs_attr_node_removename has also been
> > > modified to use the switch, and a new version of xfs_attr_remove_args
> > > consists of a simple loop to refresh the transaction until the operation
> > > is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
> > > transaction where ever the existing code used to.
> > >
> > > Calls to xfs_attr_rmtval_remove are replaced with the delay ready
> > > version __xfs_attr_rmtval_remove. We will rename
> > > __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
> > > done.
> > >
> > > xfs_attr_rmtval_remove itself is still in use by the set routines (used
> > > during a rename). For reasons of perserving existing function, we
> > > modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
> > > set. Similar to how xfs_attr_remove_args does here. Once we transition
> > > the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
> > > used and will be removed.
> > >
> > > This patch also adds a new struct xfs_delattr_context, which we will use
> > > to keep track of the current state of an attribute operation. The new
> > > xfs_delattr_state enum is used to track various operations that are in
> > > progress so that we know not to repeat them, and resume where we left
> > > off before EAGAIN was returned to cycle out the transaction. Other
> > > members take the place of local variables that need to retain their
> > > values across multiple function recalls. See xfs_attr.h for a more
> > > detailed diagram of the states.
> > >
> > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > ---
> > > fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
> > > fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
> > > fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> > > fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
> > > fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
> > > fs/xfs/xfs_attr_inactive.c | 2 +-
> > > 6 files changed, 208 insertions(+), 66 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 4b78c86..5c460f4 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> > > */
> > > STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> > > STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> > > -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> > > +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
> > > STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> > > struct xfs_da_state **state);
> > > STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> > > @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
> > > }
> > > /*
> > > + * Checks to see if a delayed attribute transaction should be rolled. If so,
> > > + * also checks for a defer finish. Transaction is finished and rolled as
> > > + * needed, and returns true of false if the delayed operation should continue.
> > > + */
> > > +bool
> > > +xfs_attr_roll_again(
> >
> > The function name suggests this is more of a status checking function
> > than one that does actual work. I'd suggest something more like
> > xfs_attr_trans_roll() based on the implementation.
> Sure, I believe Darrick had asked for this helper in the last review, so as
> long as folks are in agreement with the name, that is fine.
>
> >
> > > + struct xfs_delattr_context *dac,
> > > + int *error)
> > > +{
> > > + struct xfs_da_args *args = dac->da_args;
> > > +
> > > + if (*error != -EAGAIN)
> > > + return false;
> > > +
> > > + if (dac->flags & XFS_DAC_DEFER_FINISH) {
> > > + dac->flags &= ~XFS_DAC_DEFER_FINISH;
> > > + *error = xfs_defer_finish(&args->trans);
> > > + if (*error)
> > > + return false;
> > > + }
> > > +
> >
> > I also find the semantics of this function a little confusing. How would
> > a caller distinguish between error == -EAGAIN as passed in from the
> > caller vs. error being set by one of the transaction processing calls?
> I believe the helper was just meant to be a straight hoist from the v9
> version. The error parameter here is an input for use in this scope, it's
> not for the caller to do anything with. Other than to pipe it back in. The
> caller just needs to know whether or not to continue.
>
I don't mind having the helper and afaict it looks correct, but I find
the combination of error checking and functional change just obfuscate
the logic in the caller. I could see the reason for this kind of
implementation if it were called all over the place and we wanted to
reduce boilerplate error checking code or something, but this only has a
handful of callers. IMO this would be much cleaner as:
int xfs_attr_trans_roll(struct xfs_delattr_context *);
... and let the caller care about when to use it and what to do with
resulting errors.
> That signature is a bit odd, though this pattern does appear in a few places
> around the code, so it does help consolidate a bit. I suppose it's just a
> question of which people prefer.
>
> >
> > > + *error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > + return *error == 0;
> > > +}
> > > +
> > > +/*
> > > * Set the attribute specified in @args.
> > > */
> > > int
> > > @@ -364,23 +390,47 @@ xfs_has_attr(
> > > */
> > > int
> > > xfs_attr_remove_args(
> > > - struct xfs_da_args *args)
> > > + struct xfs_da_args *args)
> > > {
> > > - struct xfs_inode *dp = args->dp;
> > > - int error;
> > > + int error = 0;
> > > + struct xfs_delattr_context dac = {
> > > + .da_args = args,
> > > + };
> > > +
> > > + do {
> > > + error = xfs_attr_remove_iter(&dac);
> > > + } while (xfs_attr_roll_again(&dac, &error));
> > > +
> > > + return error;
> > > +}
> > > +
> > > +/*
> > > + * Remove the attribute specified in @args.
> > > + *
> > > + * This function may return -EAGAIN to signal that the transaction needs to be
> > > + * rolled. Callers should continue calling this function until they receive a
> > > + * return value other than -EAGAIN.
> > > + */
> > > +int
> > > +xfs_attr_remove_iter(
> > > + struct xfs_delattr_context *dac)
> > > +{
> > > + struct xfs_da_args *args = dac->da_args;
> > > + struct xfs_inode *dp = args->dp;
> > > +
> > > + if (dac->dela_state == XFS_DAS_RM_SHRINK)
> > > + goto node;
> >
> > Thoughts on my comments on this on the previous version?
> Apologies, I must have missed responding to it, I had a few family
> emergencies at the time and must have lost track. I do believe we need the
> goto because in the process of shrinking, it is possible that we turn back
> into a block which would cause us to fall into the wrong if/else logic with
> out it. I will run the test and double check though. I'm pretty sure I
> recall running into that though.
>
Ok. No worries, I just saw unaddressed feedback and wanted to make sure
it wasn't lost...
Brian
> >
> > > if (!xfs_inode_hasattr(dp)) {
> > > - error = -ENOATTR;
> > > + return -ENOATTR;
> > > } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
> > > ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > > - error = xfs_attr_shortform_remove(args);
> > > + return xfs_attr_shortform_remove(args);
> > > } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > > - error = xfs_attr_leaf_removename(args);
> > > - } else {
> > > - error = xfs_attr_node_removename(args);
> > > + return xfs_attr_leaf_removename(args);
> > > }
> > > -
> > > - return error;
> > > +node:
> > > + return xfs_attr_node_removename(dac);
> > > }
> > > /*
> > ...
> > > @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
> > > error = xfs_da3_join(state);
> > > if (error)
> > > goto out;
> > > - error = xfs_defer_finish(&args->trans);
> > > - if (error)
> > > - goto out;
> > > - /*
> > > - * Commit the Btree join operation and start a new trans.
> > > - */
> > > - error = xfs_trans_roll_inode(&args->trans, dp);
> > > - if (error)
> > > - goto out;
> > > +
> > > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > > + dac->dela_state = XFS_DAS_RM_SHRINK;
> > > + return -EAGAIN;
> > > }
> > > +das_rm_shrink:
> > > +
> >
> > I think I also mentioned the tail of this function might sit better in
> > the caller..
> I see, yes I'll see if I could pull that up. If I missed anything else,
> please call it out. Thank you for your reviews!
>
> Allison
>
> >
> > Brian
> >
> > > /*
> > > * If the result is small enough, push it all into the inode.
> > > */
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index 3e97a93..6c58792 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
> > > };
> > > +/*
> > > + * ========================================================================
> > > + * Structure used to pass context around among the delayed routines.
> > > + * ========================================================================
> > > + */
> > > +
> > > +/*
> > > + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
> > > + * states indicate places where the function would return -EAGAIN, and then
> > > + * immediately resume from after being recalled by the calling function. States
> > > + * marked as a "subroutine state" indicate that they belong to a subroutine, and
> > > + * so the calling function needs to pass them back to that subroutine to allow
> > > + * it to finish where it left off. But they otherwise do not have a role in the
> > > + * calling function other than just passing through.
> > > + *
> > > + * xfs_attr_remove_iter()
> > > + * XFS_DAS_RM_SHRINK ─┐
> > > + * (subroutine state) │
> > > + * └─>xfs_attr_node_removename()
> > > + * │
> > > + * v
> > > + * need to
> > > + * shrink tree? ─n─┐
> > > + * │ │
> > > + * y │
> > > + * │ │
> > > + * v │
> > > + * XFS_DAS_RM_SHRINK │
> > > + * │ │
> > > + * v │
> > > + * done <─────┘
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * Enum values for xfs_delattr_context.da_state
> > > + *
> > > + * These values are used by delayed attribute operations to keep track of where
> > > + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
> > > + * calling function to roll the transaction, and then recall the subroutine to
> > > + * finish the operation. The enum is then used by the subroutine to jump back
> > > + * to where it was and resume executing where it left off.
> > > + */
> > > +enum xfs_delattr_state {
> > > + /* Zero is uninitalized */
> > > + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
> > > +};
> > > +
> > > +/*
> > > + * Defines for xfs_delattr_context.flags
> > > + */
> > > +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
> > > +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
> > > +
> > > +/*
> > > + * Context used for keeping track of delayed attribute operations
> > > + */
> > > +struct xfs_delattr_context {
> > > + struct xfs_da_args *da_args;
> > > +
> > > + /* Used in xfs_attr_node_removename to roll through removing blocks */
> > > + struct xfs_da_state *da_state;
> > > + struct xfs_da_state_blk *blk;
> > > +
> > > + /* Used to keep track of current state of delayed operation */
> > > + unsigned int flags;
> > > + enum xfs_delattr_state dela_state;
> > > +};
> > > +
> > > /*========================================================================
> > > * Function prototypes for the kernel.
> > > *========================================================================*/
> > > @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
> > > int xfs_attr_set_args(struct xfs_da_args *args);
> > > int xfs_has_attr(struct xfs_da_args *args);
> > > int xfs_attr_remove_args(struct xfs_da_args *args);
> > > +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
> > > +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
> > > bool xfs_attr_namecheck(const void *name, size_t length);
> > > +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> > > + struct xfs_da_args *args);
> > > #endif /* __XFS_ATTR_H__ */
> > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > > index 351351c..20521bf 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > > @@ -19,8 +19,8 @@
> > > #include "xfs_bmap_btree.h"
> > > #include "xfs_bmap.h"
> > > #include "xfs_attr_sf.h"
> > > -#include "xfs_attr_remote.h"
> > > #include "xfs_attr.h"
> > > +#include "xfs_attr_remote.h"
> > > #include "xfs_attr_leaf.h"
> > > #include "xfs_error.h"
> > > #include "xfs_trace.h"
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> > > index 85dca51..20e4605 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.c
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> > > @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
> > > */
> > > int
> > > xfs_attr_rmtval_remove(
> > > - struct xfs_da_args *args)
> > > + struct xfs_da_args *args)
> > > {
> > > - xfs_dablk_t lblkno;
> > > - int blkcnt;
> > > - int error = 0;
> > > - int retval = 0;
> > > + xfs_dablk_t lblkno;
> > > + int blkcnt;
> > > + int error = 0;
> > > + struct xfs_delattr_context dac = {
> > > + .da_args = args,
> > > + };
> > > trace_xfs_attr_rmtval_remove(args);
> > > @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
> > > lblkno = args->rmtblkno;
> > > blkcnt = args->rmtblkcnt;
> > > do {
> > > - retval = __xfs_attr_rmtval_remove(args);
> > > - if (retval && retval != EAGAIN)
> > > - return retval;
> > > -
> > > - /*
> > > - * Close out trans and start the next one in the chain.
> > > - */
> > > - error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > - if (error)
> > > - return error;
> > > - } while (retval == -EAGAIN);
> > > + error = __xfs_attr_rmtval_remove(&dac);
> > > + } while (xfs_attr_roll_again(&dac, &error));
> > > - return 0;
> > > + return error;
> > > }
> > > /*
> > > @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
> > > */
> > > int
> > > __xfs_attr_rmtval_remove(
> > > - struct xfs_da_args *args)
> > > + struct xfs_delattr_context *dac)
> > > {
> > > - int error, done;
> > > + struct xfs_da_args *args = dac->da_args;
> > > + int error, done;
> > > /*
> > > * Unmap value blocks for this attr.
> > > @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
> > > if (error)
> > > return error;
> > > - error = xfs_defer_finish(&args->trans);
> > > - if (error)
> > > - return error;
> > > -
> > > - if (!done)
> > > + if (!done) {
> > > + dac->flags |= XFS_DAC_DEFER_FINISH;
> > > return -EAGAIN;
> > > + }
> > > return error;
> > > }
> > > diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
> > > index 9eee615..002fd30 100644
> > > --- a/fs/xfs/libxfs/xfs_attr_remote.h
> > > +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> > > @@ -14,5 +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_remove(struct xfs_da_args *args);
> > > +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
> > > #endif /* __XFS_ATTR_REMOTE_H__ */
> > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > > index bfad669..aaa7e66 100644
> > > --- a/fs/xfs/xfs_attr_inactive.c
> > > +++ b/fs/xfs/xfs_attr_inactive.c
> > > @@ -15,10 +15,10 @@
> > > #include "xfs_da_format.h"
> > > #include "xfs_da_btree.h"
> > > #include "xfs_inode.h"
> > > +#include "xfs_attr.h"
> > > #include "xfs_attr_remote.h"
> > > #include "xfs_trans.h"
> > > #include "xfs_bmap.h"
> > > -#include "xfs_attr.h"
> > > #include "xfs_attr_leaf.h"
> > > #include "xfs_quota.h"
> > > #include "xfs_dir2.h"
> > > --
> > > 2.7.4
> > >
> >
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v10 23/25] xfs: Add delay ready attr remove routines
2020-07-13 13:58 ` Brian Foster
@ 2020-07-15 5:04 ` Allison Collins
0 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-07-15 5:04 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On 7/13/20 6:58 AM, Brian Foster wrote:
> On Thu, Jul 09, 2020 at 03:02:08PM -0700, Allison Collins wrote:
>>
>>
>> On 7/9/20 6:24 AM, Brian Foster wrote:
>>> On Thu, Jun 25, 2020 at 04:30:16PM -0700, Allison Collins wrote:
>>>> This patch modifies the attr remove routines to be delay ready. This
>>>> means they no longer roll or commit transactions, but instead return
>>>> -EAGAIN to have the calling routine roll and refresh the transaction. In
>>>> this series, xfs_attr_remove_args has become xfs_attr_remove_iter, which
>>>> uses a sort of state machine like switch to keep track of where it was
>>>> when EAGAIN was returned. xfs_attr_node_removename has also been
>>>> modified to use the switch, and a new version of xfs_attr_remove_args
>>>> consists of a simple loop to refresh the transaction until the operation
>>>> is completed. A new XFS_DAC_DEFER_FINISH flag is used to finish the
>>>> transaction where ever the existing code used to.
>>>>
>>>> Calls to xfs_attr_rmtval_remove are replaced with the delay ready
>>>> version __xfs_attr_rmtval_remove. We will rename
>>>> __xfs_attr_rmtval_remove back to xfs_attr_rmtval_remove when we are
>>>> done.
>>>>
>>>> xfs_attr_rmtval_remove itself is still in use by the set routines (used
>>>> during a rename). For reasons of perserving existing function, we
>>>> modify xfs_attr_rmtval_remove to call xfs_defer_finish when the flag is
>>>> set. Similar to how xfs_attr_remove_args does here. Once we transition
>>>> the set routines to be delay ready, xfs_attr_rmtval_remove is no longer
>>>> used and will be removed.
>>>>
>>>> This patch also adds a new struct xfs_delattr_context, which we will use
>>>> to keep track of the current state of an attribute operation. The new
>>>> xfs_delattr_state enum is used to track various operations that are in
>>>> progress so that we know not to repeat them, and resume where we left
>>>> off before EAGAIN was returned to cycle out the transaction. Other
>>>> members take the place of local variables that need to retain their
>>>> values across multiple function recalls. See xfs_attr.h for a more
>>>> detailed diagram of the states.
>>>>
>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>> ---
>>>> fs/xfs/libxfs/xfs_attr.c | 155 ++++++++++++++++++++++++++++++----------
>>>> fs/xfs/libxfs/xfs_attr.h | 73 +++++++++++++++++++
>>>> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
>>>> fs/xfs/libxfs/xfs_attr_remote.c | 40 +++++------
>>>> fs/xfs/libxfs/xfs_attr_remote.h | 2 +-
>>>> fs/xfs/xfs_attr_inactive.c | 2 +-
>>>> 6 files changed, 208 insertions(+), 66 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>> index 4b78c86..5c460f4 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>> @@ -53,7 +53,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>>> */
>>>> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>>> STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>>> -STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>>>> +STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
>>>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>>> struct xfs_da_state **state);
>>>> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>>>> @@ -264,6 +264,32 @@ xfs_attr_set_shortform(
>>>> }
>>>> /*
>>>> + * Checks to see if a delayed attribute transaction should be rolled. If so,
>>>> + * also checks for a defer finish. Transaction is finished and rolled as
>>>> + * needed, and returns true of false if the delayed operation should continue.
>>>> + */
>>>> +bool
>>>> +xfs_attr_roll_again(
>>>
>>> The function name suggests this is more of a status checking function
>>> than one that does actual work. I'd suggest something more like
>>> xfs_attr_trans_roll() based on the implementation.
>> Sure, I believe Darrick had asked for this helper in the last review, so as
>> long as folks are in agreement with the name, that is fine.
>>
>>>
>>>> + struct xfs_delattr_context *dac,
>>>> + int *error)
>>>> +{
>>>> + struct xfs_da_args *args = dac->da_args;
>>>> +
>>>> + if (*error != -EAGAIN)
>>>> + return false;
>>>> +
>>>> + if (dac->flags & XFS_DAC_DEFER_FINISH) {
>>>> + dac->flags &= ~XFS_DAC_DEFER_FINISH;
>>>> + *error = xfs_defer_finish(&args->trans);
>>>> + if (*error)
>>>> + return false;
>>>> + }
>>>> +
>>>
>>> I also find the semantics of this function a little confusing. How would
>>> a caller distinguish between error == -EAGAIN as passed in from the
>>> caller vs. error being set by one of the transaction processing calls?
>> I believe the helper was just meant to be a straight hoist from the v9
>> version. The error parameter here is an input for use in this scope, it's
>> not for the caller to do anything with. Other than to pipe it back in. The
>> caller just needs to know whether or not to continue.
>>
>
> I don't mind having the helper and afaict it looks correct, but I find
> the combination of error checking and functional change just obfuscate
> the logic in the caller. I could see the reason for this kind of
> implementation if it were called all over the place and we wanted to
> reduce boilerplate error checking code or something, but this only has a
> handful of callers. IMO this would be much cleaner as:
>
> int xfs_attr_trans_roll(struct xfs_delattr_context *);
>
> ... and let the caller care about when to use it and what to do with
> resulting errors.
Sure I am fine with that as long as everyone else is. Unless I hear
otherwise, I will pull the error handling into the caller.
>
>> That signature is a bit odd, though this pattern does appear in a few places
>> around the code, so it does help consolidate a bit. I suppose it's just a
>> question of which people prefer.
>>
>>>
>>>> + *error = xfs_trans_roll_inode(&args->trans, args->dp);
>>>> + return *error == 0;
>>>> +}
>>>> +
>>>> +/*
>>>> * Set the attribute specified in @args.
>>>> */
>>>> int
>>>> @@ -364,23 +390,47 @@ xfs_has_attr(
>>>> */
>>>> int
>>>> xfs_attr_remove_args(
>>>> - struct xfs_da_args *args)
>>>> + struct xfs_da_args *args)
>>>> {
>>>> - struct xfs_inode *dp = args->dp;
>>>> - int error;
>>>> + int error = 0;
>>>> + struct xfs_delattr_context dac = {
>>>> + .da_args = args,
>>>> + };
>>>> +
>>>> + do {
>>>> + error = xfs_attr_remove_iter(&dac);
>>>> + } while (xfs_attr_roll_again(&dac, &error));
>>>> +
>>>> + return error;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Remove the attribute specified in @args.
>>>> + *
>>>> + * This function may return -EAGAIN to signal that the transaction needs to be
>>>> + * rolled. Callers should continue calling this function until they receive a
>>>> + * return value other than -EAGAIN.
>>>> + */
>>>> +int
>>>> +xfs_attr_remove_iter(
>>>> + struct xfs_delattr_context *dac)
>>>> +{
>>>> + struct xfs_da_args *args = dac->da_args;
>>>> + struct xfs_inode *dp = args->dp;
>>>> +
>>>> + if (dac->dela_state == XFS_DAS_RM_SHRINK)
>>>> + goto node;
>>>
>>> Thoughts on my comments on this on the previous version?
>> Apologies, I must have missed responding to it, I had a few family
>> emergencies at the time and must have lost track. I do believe we need the
>> goto because in the process of shrinking, it is possible that we turn back
>> into a block which would cause us to fall into the wrong if/else logic with
>> out it. I will run the test and double check though. I'm pretty sure I
>> recall running into that though.
>>
>
> Ok. No worries, I just saw unaddressed feedback and wanted to make sure
> it wasn't lost...
>
> Brian
Yep, I checked this part, we do need the goto, or we do indeed get stuck
in the wrong code path on re-entry. In any case, will adjust the helper
function mentioned above. Thanks for the reviews!
Allison
>
>>>
>>>> if (!xfs_inode_hasattr(dp)) {
>>>> - error = -ENOATTR;
>>>> + return -ENOATTR;
>>>> } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) {
>>>> ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
>>>> - error = xfs_attr_shortform_remove(args);
>>>> + return xfs_attr_shortform_remove(args);
>>>> } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>>> - error = xfs_attr_leaf_removename(args);
>>>> - } else {
>>>> - error = xfs_attr_node_removename(args);
>>>> + return xfs_attr_leaf_removename(args);
>>>> }
>>>> -
>>>> - return error;
>>>> +node:
>>>> + return xfs_attr_node_removename(dac);
>>>> }
>>>> /*
>>> ...
>>>> @@ -1268,17 +1348,14 @@ xfs_attr_node_removename(
>>>> error = xfs_da3_join(state);
>>>> if (error)
>>>> goto out;
>>>> - error = xfs_defer_finish(&args->trans);
>>>> - if (error)
>>>> - goto out;
>>>> - /*
>>>> - * Commit the Btree join operation and start a new trans.
>>>> - */
>>>> - error = xfs_trans_roll_inode(&args->trans, dp);
>>>> - if (error)
>>>> - goto out;
>>>> +
>>>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>>>> + dac->dela_state = XFS_DAS_RM_SHRINK;
>>>> + return -EAGAIN;
>>>> }
>>>> +das_rm_shrink:
>>>> +
>>>
>>> I think I also mentioned the tail of this function might sit better in
>>> the caller..
>> I see, yes I'll see if I could pull that up. If I missed anything else,
>> please call it out. Thank you for your reviews!
>>
>> Allison
>>
>>>
>>> Brian
>>>
>>>> /*
>>>> * If the result is small enough, push it all into the inode.
>>>> */
>>>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>>>> index 3e97a93..6c58792 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr.h
>>>> +++ b/fs/xfs/libxfs/xfs_attr.h
>>>> @@ -74,6 +74,75 @@ struct xfs_attr_list_context {
>>>> };
>>>> +/*
>>>> + * ========================================================================
>>>> + * Structure used to pass context around among the delayed routines.
>>>> + * ========================================================================
>>>> + */
>>>> +
>>>> +/*
>>>> + * Below is a state machine diagram for attr remove operations. The XFS_DAS_*
>>>> + * states indicate places where the function would return -EAGAIN, and then
>>>> + * immediately resume from after being recalled by the calling function. States
>>>> + * marked as a "subroutine state" indicate that they belong to a subroutine, and
>>>> + * so the calling function needs to pass them back to that subroutine to allow
>>>> + * it to finish where it left off. But they otherwise do not have a role in the
>>>> + * calling function other than just passing through.
>>>> + *
>>>> + * xfs_attr_remove_iter()
>>>> + * XFS_DAS_RM_SHRINK ─┐
>>>> + * (subroutine state) │
>>>> + * └─>xfs_attr_node_removename()
>>>> + * │
>>>> + * v
>>>> + * need to
>>>> + * shrink tree? ─n─┐
>>>> + * │ │
>>>> + * y │
>>>> + * │ │
>>>> + * v │
>>>> + * XFS_DAS_RM_SHRINK │
>>>> + * │ │
>>>> + * v │
>>>> + * done <─────┘
>>>> + *
>>>> + */
>>>> +
>>>> +/*
>>>> + * Enum values for xfs_delattr_context.da_state
>>>> + *
>>>> + * These values are used by delayed attribute operations to keep track of where
>>>> + * they were before they returned -EAGAIN. A return code of -EAGAIN signals the
>>>> + * calling function to roll the transaction, and then recall the subroutine to
>>>> + * finish the operation. The enum is then used by the subroutine to jump back
>>>> + * to where it was and resume executing where it left off.
>>>> + */
>>>> +enum xfs_delattr_state {
>>>> + /* Zero is uninitalized */
>>>> + XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
>>>> +};
>>>> +
>>>> +/*
>>>> + * Defines for xfs_delattr_context.flags
>>>> + */
>>>> +#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
>>>> +#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
>>>> +
>>>> +/*
>>>> + * Context used for keeping track of delayed attribute operations
>>>> + */
>>>> +struct xfs_delattr_context {
>>>> + struct xfs_da_args *da_args;
>>>> +
>>>> + /* Used in xfs_attr_node_removename to roll through removing blocks */
>>>> + struct xfs_da_state *da_state;
>>>> + struct xfs_da_state_blk *blk;
>>>> +
>>>> + /* Used to keep track of current state of delayed operation */
>>>> + unsigned int flags;
>>>> + enum xfs_delattr_state dela_state;
>>>> +};
>>>> +
>>>> /*========================================================================
>>>> * Function prototypes for the kernel.
>>>> *========================================================================*/
>>>> @@ -91,6 +160,10 @@ int xfs_attr_set(struct xfs_da_args *args);
>>>> int xfs_attr_set_args(struct xfs_da_args *args);
>>>> int xfs_has_attr(struct xfs_da_args *args);
>>>> int xfs_attr_remove_args(struct xfs_da_args *args);
>>>> +int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
>>>> +bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
>>>> bool xfs_attr_namecheck(const void *name, size_t length);
>>>> +void xfs_delattr_context_init(struct xfs_delattr_context *dac,
>>>> + struct xfs_da_args *args);
>>>> #endif /* __XFS_ATTR_H__ */
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> index 351351c..20521bf 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>>>> @@ -19,8 +19,8 @@
>>>> #include "xfs_bmap_btree.h"
>>>> #include "xfs_bmap.h"
>>>> #include "xfs_attr_sf.h"
>>>> -#include "xfs_attr_remote.h"
>>>> #include "xfs_attr.h"
>>>> +#include "xfs_attr_remote.h"
>>>> #include "xfs_attr_leaf.h"
>>>> #include "xfs_error.h"
>>>> #include "xfs_trace.h"
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> index 85dca51..20e4605 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>>>> @@ -676,12 +676,14 @@ xfs_attr_rmtval_invalidate(
>>>> */
>>>> int
>>>> xfs_attr_rmtval_remove(
>>>> - struct xfs_da_args *args)
>>>> + struct xfs_da_args *args)
>>>> {
>>>> - xfs_dablk_t lblkno;
>>>> - int blkcnt;
>>>> - int error = 0;
>>>> - int retval = 0;
>>>> + xfs_dablk_t lblkno;
>>>> + int blkcnt;
>>>> + int error = 0;
>>>> + struct xfs_delattr_context dac = {
>>>> + .da_args = args,
>>>> + };
>>>> trace_xfs_attr_rmtval_remove(args);
>>>> @@ -691,19 +693,10 @@ xfs_attr_rmtval_remove(
>>>> lblkno = args->rmtblkno;
>>>> blkcnt = args->rmtblkcnt;
>>>> do {
>>>> - retval = __xfs_attr_rmtval_remove(args);
>>>> - if (retval && retval != EAGAIN)
>>>> - return retval;
>>>> -
>>>> - /*
>>>> - * Close out trans and start the next one in the chain.
>>>> - */
>>>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>>>> - if (error)
>>>> - return error;
>>>> - } while (retval == -EAGAIN);
>>>> + error = __xfs_attr_rmtval_remove(&dac);
>>>> + } while (xfs_attr_roll_again(&dac, &error));
>>>> - return 0;
>>>> + return error;
>>>> }
>>>> /*
>>>> @@ -713,9 +706,10 @@ xfs_attr_rmtval_remove(
>>>> */
>>>> int
>>>> __xfs_attr_rmtval_remove(
>>>> - struct xfs_da_args *args)
>>>> + struct xfs_delattr_context *dac)
>>>> {
>>>> - int error, done;
>>>> + struct xfs_da_args *args = dac->da_args;
>>>> + int error, done;
>>>> /*
>>>> * Unmap value blocks for this attr.
>>>> @@ -725,12 +719,10 @@ __xfs_attr_rmtval_remove(
>>>> if (error)
>>>> return error;
>>>> - error = xfs_defer_finish(&args->trans);
>>>> - if (error)
>>>> - return error;
>>>> -
>>>> - if (!done)
>>>> + if (!done) {
>>>> + dac->flags |= XFS_DAC_DEFER_FINISH;
>>>> return -EAGAIN;
>>>> + }
>>>> return error;
>>>> }
>>>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
>>>> index 9eee615..002fd30 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr_remote.h
>>>> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
>>>> @@ -14,5 +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_remove(struct xfs_da_args *args);
>>>> +int __xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
>>>> #endif /* __XFS_ATTR_REMOTE_H__ */
>>>> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
>>>> index bfad669..aaa7e66 100644
>>>> --- a/fs/xfs/xfs_attr_inactive.c
>>>> +++ b/fs/xfs/xfs_attr_inactive.c
>>>> @@ -15,10 +15,10 @@
>>>> #include "xfs_da_format.h"
>>>> #include "xfs_da_btree.h"
>>>> #include "xfs_inode.h"
>>>> +#include "xfs_attr.h"
>>>> #include "xfs_attr_remote.h"
>>>> #include "xfs_trans.h"
>>>> #include "xfs_bmap.h"
>>>> -#include "xfs_attr.h"
>>>> #include "xfs_attr_leaf.h"
>>>> #include "xfs_quota.h"
>>>> #include "xfs_dir2.h"
>>>> --
>>>> 2.7.4
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v10 24/25] xfs: Add delay ready attr set routines
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (22 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 23/25] xfs: Add delay ready attr remove routines Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
2020-06-25 23:30 ` [PATCH v10 25/25] xfs: Rename __xfs_attr_rmtval_remove Allison Collins
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
This patch modifies the attr set routines to be delay ready. This means
they no longer roll or commit transactions, but instead return -EAGAIN
to have the calling routine roll and refresh the transaction. In this
series, xfs_attr_set_args has become xfs_attr_set_iter, which uses a
state machine like switch to keep track of where it was when EAGAIN was
returned. See xfs_attr.h for a more detailed diagram of the states.
Two new helper functions have been added: xfs_attr_rmtval_set_init and
xfs_attr_rmtval_set_blk. They provide a subset of logic similar to
xfs_attr_rmtval_set, but they store the current block in the delay attr
context to allow the caller to roll the transaction between allocations.
This helps to simplify and consolidate code used by
xfs_attr_leaf_addname and xfs_attr_node_addname. xfs_attr_set_args has
now become a simple loop to refresh the transaction until the operation
is completed. Lastly, xfs_attr_rmtval_remove is no longer used, and is
removed.
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 366 ++++++++++++++++++++++++++--------------
fs/xfs/libxfs/xfs_attr.h | 126 +++++++++++++-
fs/xfs/libxfs/xfs_attr_remote.c | 96 +++++++----
fs/xfs/libxfs/xfs_attr_remote.h | 4 +
fs/xfs/xfs_trace.h | 1 -
5 files changed, 438 insertions(+), 155 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 5c460f4..2d200e3 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -44,7 +44,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
* Internal routines when attribute list is one block.
*/
STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
-STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
+STATIC int xfs_attr_leaf_addname(struct xfs_delattr_context *dac);
STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
@@ -52,12 +52,15 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
* Internal routines when attribute list is more than one block.
*/
STATIC int xfs_attr_node_get(xfs_da_args_t *args);
-STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
+STATIC int xfs_attr_node_addname(struct xfs_delattr_context *dac);
STATIC int xfs_attr_node_removename(struct xfs_delattr_context *dac);
STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
struct xfs_da_state **state);
STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
+STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
+STATIC int xfs_attr_set_iter(struct xfs_delattr_context *dac,
+ struct xfs_buf **leaf_bp);
int
xfs_inode_hasattr(
@@ -218,8 +221,11 @@ xfs_attr_is_shortform(
/*
* Attempts to set an attr in shortform, or converts short form to leaf form if
- * there is not enough room. If the attr is set, the transaction is committed
- * and set to NULL.
+ * there is not enough room. This function is meant to operate as a helper
+ * routine to the delayed attribute functions. It returns -EAGAIN to indicate
+ * that the calling function should roll the transaction, and then proceed to
+ * add the attr in leaf form. This subroutine does not expect to be recalled
+ * again like the other delayed attr routines do.
*/
STATIC int
xfs_attr_set_shortform(
@@ -227,16 +233,16 @@ xfs_attr_set_shortform(
struct xfs_buf **leaf_bp)
{
struct xfs_inode *dp = args->dp;
- int error, error2 = 0;
+ int error = 0;
/*
* Try to add the attr to the attribute list in the inode.
*/
error = xfs_attr_try_sf_addname(dp, args);
+
+ /* Should only be 0, -EEXIST or ENOSPC */
if (error != -ENOSPC) {
- error2 = xfs_trans_commit(args->trans);
- args->trans = NULL;
- return error ? error : error2;
+ return error;
}
/*
* It won't fit in the shortform, transform to a leaf block. GROT:
@@ -249,18 +255,10 @@ xfs_attr_set_shortform(
/*
* Prevent the leaf buffer from being unlocked so that a concurrent AIL
* push cannot grab the half-baked leaf buffer and run into problems
- * with the write verifier. Once we're done rolling the transaction we
- * can release the hold and add the attr to the leaf.
+ * with the write verifier.
*/
xfs_trans_bhold(args->trans, *leaf_bp);
- error = xfs_defer_finish(&args->trans);
- xfs_trans_bhold_release(args->trans, *leaf_bp);
- if (error) {
- xfs_trans_brelse(args->trans, *leaf_bp);
- return error;
- }
-
- return 0;
+ return -EAGAIN;
}
/*
@@ -268,7 +266,7 @@ xfs_attr_set_shortform(
* also checks for a defer finish. Transaction is finished and rolled as
* needed, and returns true of false if the delayed operation should continue.
*/
-bool
+STATIC bool
xfs_attr_roll_again(
struct xfs_delattr_context *dac,
int *error)
@@ -296,61 +294,126 @@ int
xfs_attr_set_args(
struct xfs_da_args *args)
{
- struct xfs_inode *dp = args->dp;
- struct xfs_buf *leaf_bp = NULL;
- int error = 0;
+ struct xfs_buf *leaf_bp = NULL;
+ int error = 0;
+ struct xfs_delattr_context dac = {
+ .da_args = args,
+ };
+
+ do {
+ error = xfs_attr_set_iter(&dac, &leaf_bp);
+ if (!xfs_attr_roll_again(&dac, &error))
+ break;
+
+ if (leaf_bp) {
+ xfs_trans_bjoin(args->trans, leaf_bp);
+ xfs_trans_bhold(args->trans, leaf_bp);
+ }
+
+ } while (true);
+
+ return error;
+}
+
+/*
+ * Set the attribute specified in @args.
+ * This routine is meant to function as a delayed operation, and may return
+ * -EAGAIN when the transaction needs to be rolled. Calling functions will need
+ * to handle this, and recall the function until a successful error code is
+ * returned.
+ */
+STATIC int
+xfs_attr_set_iter(
+ struct xfs_delattr_context *dac,
+ struct xfs_buf **leaf_bp)
+{
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_inode *dp = args->dp;
+ int error = 0;
+
+ /* State machine switch */
+ switch (dac->dela_state) {
+ case XFS_DAS_FLIP_LFLAG:
+ case XFS_DAS_FOUND_LBLK:
+ goto das_leaf;
+ case XFS_DAS_FOUND_NBLK:
+ case XFS_DAS_FLIP_NFLAG:
+ case XFS_DAS_ALLOC_NODE:
+ goto das_node;
+ default:
+ break;
+ }
/*
* If the attribute list is already in leaf format, jump straight to
* leaf handling. Otherwise, try to add the attribute to the shortform
* list; if there's no room then convert the list to leaf format and try
- * again.
+ * again. No need to set state as we will be in leaf form when we come
+ * back
*/
if (xfs_attr_is_shortform(dp)) {
/*
- * If the attr was successfully set in shortform, the
- * transaction is committed and set to NULL. Otherwise, is it
- * converted from shortform to leaf, and the transaction is
- * retained.
+ * If the attr was successfully set in shortform, no need to
+ * continue. Otherwise, is it converted from shortform to leaf
+ * and -EAGAIN is returned.
*/
- error = xfs_attr_set_shortform(args, &leaf_bp);
- if (error || !args->trans)
- return error;
+ error = xfs_attr_set_shortform(args, leaf_bp);
+ if (error == -EAGAIN)
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+
+ return error;
}
- if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
- error = xfs_attr_leaf_addname(args);
- if (error != -ENOSPC)
- return error;
+ /*
+ * After a shortform to leaf conversion, we need to hold the leaf and
+ * cycle out the transaction. When we get back, we need to release
+ * the leaf.
+ */
+ if (*leaf_bp != NULL) {
+ xfs_trans_bhold_release(args->trans, *leaf_bp);
+ *leaf_bp = NULL;
+ }
- /*
- * Promote the attribute list to the Btree format.
- */
- error = xfs_attr3_leaf_to_node(args);
- if (error)
- return error;
+ if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+ error = xfs_attr_leaf_try_add(args, *leaf_bp);
+ switch (error) {
+ case -ENOSPC:
+ /*
+ * Promote the attribute list to the Btree format.
+ */
+ error = xfs_attr3_leaf_to_node(args);
+ if (error)
+ return error;
- /*
- * Finish any deferred work items and roll the transaction once
- * more. The goal here is to call node_addname with the inode
- * and transaction in the same state (inode locked and joined,
- * transaction clean) no matter how we got to this step.
- */
- error = xfs_defer_finish(&args->trans);
- if (error)
+ /*
+ * Finish any deferred work items and roll the
+ * transaction once more. The goal here is to call
+ * node_addname with the inode and transaction in the
+ * same state (inode locked and joined, transaction
+ * clean) no matter how we got to this step.
+ */
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+ return -EAGAIN;
+ case 0:
+ dac->dela_state = XFS_DAS_FOUND_LBLK;
+ return -EAGAIN;
+ default:
return error;
+ }
+das_leaf:
+ error = xfs_attr_leaf_addname(dac);
+ if (error == -ENOSPC)
+ /*
+ * No need to set state. We will be in node form when
+ * we are recalled
+ */
+ return -EAGAIN;
- /*
- * Commit the current trans (including the inode) and
- * start a new one.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- return error;
+ return error;
}
-
- error = xfs_attr_node_addname(args);
+das_node:
+ error = xfs_attr_node_addname(dac);
return error;
}
@@ -707,28 +770,30 @@ xfs_attr_leaf_try_add(
*
* This leaf block cannot have a "remote" value, we only call this routine
* if bmap_one_block() says there is only one block (ie: no remote blks).
+ *
+ * This routine is meant to function as a delayed operation, and may return
+ * -EAGAIN when the transaction needs to be rolled. Calling functions will need
+ * to handle this, and recall the function until a successful error code is
+ * returned.
*/
STATIC int
xfs_attr_leaf_addname(
- struct xfs_da_args *args)
+ struct xfs_delattr_context *dac)
{
- int error, forkoff;
- struct xfs_buf *bp = NULL;
- struct xfs_inode *dp = args->dp;
-
- trace_xfs_attr_leaf_addname(args);
-
- error = xfs_attr_leaf_try_add(args, bp);
- if (error)
- return error;
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_buf *bp = NULL;
+ int error, forkoff;
+ struct xfs_inode *dp = args->dp;
- /*
- * Commit the transaction that added the attr name so that
- * later routines can manage their own transactions.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- return error;
+ /* State machine switch */
+ switch (dac->dela_state) {
+ case XFS_DAS_FLIP_LFLAG:
+ goto das_flip_flag;
+ case XFS_DAS_RM_LBLK:
+ goto das_rm_lblk;
+ default:
+ break;
+ }
/*
* If there was an out-of-line value, allocate the blocks we
@@ -736,12 +801,34 @@ xfs_attr_leaf_addname(
* after we create the attribute so that we don't overflow the
* maximum size of a transaction and/or hit a deadlock.
*/
- if (args->rmtblkno > 0) {
- error = xfs_attr_rmtval_set(args);
+
+ /* Open coded xfs_attr_rmtval_set without trans handling */
+ if ((dac->flags & XFS_DAC_LEAF_ADDNAME_INIT) == 0) {
+ dac->flags |= XFS_DAC_LEAF_ADDNAME_INIT;
+ if (args->rmtblkno > 0) {
+ error = xfs_attr_rmtval_find_space(dac);
+ if (error)
+ return error;
+ }
+ }
+
+ /*
+ * Roll through the "value", allocating blocks on disk as
+ * required.
+ */
+ if (dac->blkcnt > 0) {
+ error = xfs_attr_rmtval_set_blk(dac);
if (error)
return error;
+
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+ return -EAGAIN;
}
+ error = xfs_attr_rmtval_set_value(args);
+ if (error)
+ return error;
+
if (!(args->op_flags & XFS_DA_OP_RENAME)) {
/*
* Added a "remote" value, just clear the incomplete flag.
@@ -761,29 +848,29 @@ xfs_attr_leaf_addname(
* In a separate transaction, set the incomplete flag on the "old" attr
* and clear the incomplete flag on the "new" attr.
*/
-
error = xfs_attr3_leaf_flipflags(args);
if (error)
return error;
/*
* Commit the flag value change and start the next trans in series.
*/
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- return error;
-
+ dac->dela_state = XFS_DAS_FLIP_LFLAG;
+ return -EAGAIN;
+das_flip_flag:
/*
* Dismantle the "old" attribute/value pair by removing a "remote" value
* (if it exists).
*/
xfs_attr_restore_rmt_blk(args);
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
+das_rm_lblk:
if (args->rmtblkno) {
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
-
- error = xfs_attr_rmtval_remove(args);
+ error = __xfs_attr_rmtval_remove(dac);
+ if (error == -EAGAIN)
+ dac->dela_state = XFS_DAS_RM_LBLK;
if (error)
return error;
}
@@ -952,16 +1039,23 @@ xfs_attr_node_hasname(
*
* "Remote" attribute values confuse the issue and atomic rename operations
* add a whole extra layer of confusion on top of that.
+ *
+ * This routine is meant to function as a delayed operation, and may return
+ * -EAGAIN when the transaction needs to be rolled. Calling functions will need
+ * to handle this, and recall the function until a successful error code is
+ *returned.
*/
STATIC int
xfs_attr_node_addname(
- struct xfs_da_args *args)
+ struct xfs_delattr_context *dac)
{
- struct xfs_da_state *state;
- struct xfs_da_state_blk *blk;
- struct xfs_inode *dp;
- struct xfs_mount *mp;
- int retval, error;
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_da_state *state = NULL;
+ struct xfs_da_state_blk *blk;
+ struct xfs_inode *dp;
+ struct xfs_mount *mp;
+ int retval = 0;
+ int error = 0;
trace_xfs_attr_node_addname(args);
@@ -970,7 +1064,21 @@ xfs_attr_node_addname(
*/
dp = args->dp;
mp = dp->i_mount;
-restart:
+
+ /* State machine switch */
+ switch (dac->dela_state) {
+ case XFS_DAS_FLIP_NFLAG:
+ goto das_flip_flag;
+ case XFS_DAS_FOUND_NBLK:
+ goto das_found_nblk;
+ case XFS_DAS_ALLOC_NODE:
+ goto das_alloc_node;
+ case XFS_DAS_RM_NBLK:
+ goto das_rm_nblk;
+ default:
+ break;
+ }
+
/*
* Search to see if name already exists, and get back a pointer
* to where it should go.
@@ -1016,19 +1124,13 @@ xfs_attr_node_addname(
error = xfs_attr3_leaf_to_node(args);
if (error)
goto out;
- error = xfs_defer_finish(&args->trans);
- if (error)
- goto out;
/*
- * Commit the node conversion and start the next
- * trans in the chain.
+ * Restart routine from the top. No need to set the
+ * state
*/
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- goto out;
-
- goto restart;
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+ return -EAGAIN;
}
/*
@@ -1040,9 +1142,7 @@ xfs_attr_node_addname(
error = xfs_da3_split(state);
if (error)
goto out;
- error = xfs_defer_finish(&args->trans);
- if (error)
- goto out;
+ dac->flags |= XFS_DAC_DEFER_FINISH;
} else {
/*
* Addition succeeded, update Btree hashvals.
@@ -1057,13 +1157,9 @@ xfs_attr_node_addname(
xfs_da_state_free(state);
state = NULL;
- /*
- * Commit the leaf addition or btree split and start the next
- * trans in the chain.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- goto out;
+ dac->dela_state = XFS_DAS_FOUND_NBLK;
+ return -EAGAIN;
+das_found_nblk:
/*
* If there was an out-of-line value, allocate the blocks we
@@ -1072,7 +1168,27 @@ xfs_attr_node_addname(
* maximum size of a transaction and/or hit a deadlock.
*/
if (args->rmtblkno > 0) {
- error = xfs_attr_rmtval_set(args);
+ /* Open coded xfs_attr_rmtval_set without trans handling */
+ error = xfs_attr_rmtval_find_space(dac);
+ if (error)
+ return error;
+
+ /*
+ * Roll through the "value", allocating blocks on disk as
+ * required.
+ */
+das_alloc_node:
+ if (dac->blkcnt > 0) {
+ error = xfs_attr_rmtval_set_blk(dac);
+ if (error)
+ return error;
+
+ dac->flags |= XFS_DAC_DEFER_FINISH;
+ dac->dela_state = XFS_DAS_ALLOC_NODE;
+ return -EAGAIN;
+ }
+
+ error = xfs_attr_rmtval_set_value(args);
if (error)
return error;
}
@@ -1102,22 +1218,28 @@ xfs_attr_node_addname(
/*
* Commit the flag value change and start the next trans in series
*/
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- goto out;
-
+ dac->dela_state = XFS_DAS_FLIP_NFLAG;
+ return -EAGAIN;
+das_flip_flag:
/*
* Dismantle the "old" attribute/value pair by removing a "remote" value
* (if it exists).
*/
xfs_attr_restore_rmt_blk(args);
+ error = xfs_attr_rmtval_invalidate(args);
+ if (error)
+ return error;
+
+das_rm_nblk:
if (args->rmtblkno) {
- error = xfs_attr_rmtval_invalidate(args);
- if (error)
- return error;
+ error = __xfs_attr_rmtval_remove(dac);
+
+ if (error == -EAGAIN) {
+ dac->dela_state = XFS_DAS_RM_NBLK;
+ return -EAGAIN;
+ }
- error = xfs_attr_rmtval_remove(args);
if (error)
return error;
}
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 6c58792..4f6bba8 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -106,6 +106,118 @@ struct xfs_attr_list_context {
* v │
* done <─────┘
*
+ *
+ * Below is a state machine diagram for attr set operations.
+ *
+ * xfs_attr_set_iter()
+ * │
+ * v
+ * ┌───n── fork has
+ * │ only 1 blk?
+ * │ │
+ * │ y
+ * │ │
+ * │ v
+ * │ xfs_attr_leaf_try_add()
+ * │ │
+ * │ v
+ * │ had enough
+ * ├───n────space?
+ * │ │
+ * │ y
+ * │ │
+ * │ v
+ * │ XFS_DAS_FOUND_LBLK ──┐
+ * │ │
+ * │ XFS_DAS_FLIP_LFLAG ──┤
+ * │ (subroutine state) │
+ * │ │
+ * │ └─>xfs_attr_leaf_addname()
+ * │ │
+ * │ v
+ * │ was this
+ * │ a rename? ──n─┐
+ * │ │ │
+ * │ y │
+ * │ │ │
+ * │ v │
+ * │ flip incomplete │
+ * │ flag │
+ * │ │ │
+ * │ v │
+ * │ XFS_DAS_FLIP_LFLAG │
+ * │ │ │
+ * │ v │
+ * │ remove │
+ * │ XFS_DAS_RM_LBLK ─> old name │
+ * │ ^ │ │
+ * │ │ v │
+ * │ └──────y── more to │
+ * │ remove │
+ * │ │ │
+ * │ n │
+ * │ │ │
+ * │ v │
+ * │ done <──────┘
+ * └──> XFS_DAS_FOUND_NBLK ──┐
+ * (subroutine state) │
+ * │
+ * XFS_DAS_ALLOC_NODE ──┤
+ * (subroutine state) │
+ * │
+ * XFS_DAS_FLIP_NFLAG ──┤
+ * (subroutine state) │
+ * │
+ * └─>xfs_attr_node_addname()
+ * │
+ * v
+ * find space to store
+ * attr. Split if needed
+ * │
+ * v
+ * XFS_DAS_FOUND_NBLK
+ * │
+ * v
+ * ┌─────n── need to
+ * │ alloc blks?
+ * │ │
+ * │ y
+ * │ │
+ * │ v
+ * │ ┌─>XFS_DAS_ALLOC_NODE
+ * │ │ │
+ * │ │ v
+ * │ └──y── need to alloc
+ * │ more blocks?
+ * │ │
+ * │ n
+ * │ │
+ * │ v
+ * │ was this
+ * └────────> a rename? ──n─┐
+ * │ │
+ * y │
+ * │ │
+ * v │
+ * flip incomplete │
+ * flag │
+ * │ │
+ * v │
+ * XFS_DAS_FLIP_NFLAG │
+ * │ │
+ * v │
+ * remove │
+ * XFS_DAS_RM_NBLK ─> old name │
+ * ^ │ │
+ * │ v │
+ * └──────y── more to │
+ * remove │
+ * │ │
+ * n │
+ * │ │
+ * v │
+ * done <──────┘
+ *
*/
/*
@@ -120,6 +232,13 @@ struct xfs_attr_list_context {
enum xfs_delattr_state {
/* Zero is uninitalized */
XFS_DAS_RM_SHRINK = 1, /* We are shrinking the tree */
+ XFS_DAS_FOUND_LBLK, /* We found leaf blk for attr */
+ XFS_DAS_FOUND_NBLK, /* We found node blk for attr */
+ XFS_DAS_FLIP_LFLAG, /* Flipped leaf INCOMPLETE attr flag */
+ XFS_DAS_RM_LBLK, /* A rename is removing leaf blocks */
+ XFS_DAS_ALLOC_NODE, /* We are allocating node blocks */
+ XFS_DAS_FLIP_NFLAG, /* Flipped node INCOMPLETE attr flag */
+ XFS_DAS_RM_NBLK, /* A rename is removing node blocks */
};
/*
@@ -127,6 +246,7 @@ enum xfs_delattr_state {
*/
#define XFS_DAC_DEFER_FINISH 0x01 /* finish the transaction */
#define XFS_DAC_NODE_RMVNAME_INIT 0x02 /* xfs_attr_node_removename init */
+#define XFS_DAC_LEAF_ADDNAME_INIT 0x04 /* xfs_attr_leaf_addname init*/
/*
* Context used for keeping track of delayed attribute operations
@@ -134,6 +254,11 @@ enum xfs_delattr_state {
struct xfs_delattr_context {
struct xfs_da_args *da_args;
+ /* Used in xfs_attr_rmtval_set_blk to roll through allocating blocks */
+ struct xfs_bmbt_irec map;
+ xfs_dablk_t lblkno;
+ int blkcnt;
+
/* Used in xfs_attr_node_removename to roll through removing blocks */
struct xfs_da_state *da_state;
struct xfs_da_state_blk *blk;
@@ -161,7 +286,6 @@ int xfs_attr_set_args(struct xfs_da_args *args);
int xfs_has_attr(struct xfs_da_args *args);
int xfs_attr_remove_args(struct xfs_da_args *args);
int xfs_attr_remove_iter(struct xfs_delattr_context *dac);
-bool xfs_attr_roll_again(struct xfs_delattr_context *dac, int *error);
bool xfs_attr_namecheck(const void *name, size_t length);
void xfs_delattr_context_init(struct xfs_delattr_context *dac,
struct xfs_da_args *args);
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 20e4605..ceaefb3 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -443,7 +443,7 @@ xfs_attr_rmtval_get(
* Find a "hole" in the attribute address space large enough for us to drop the
* new attribute's value into
*/
-STATIC int
+int
xfs_attr_rmt_find_hole(
struct xfs_da_args *args)
{
@@ -470,7 +470,7 @@ xfs_attr_rmt_find_hole(
return 0;
}
-STATIC int
+int
xfs_attr_rmtval_set_value(
struct xfs_da_args *args)
{
@@ -630,6 +630,69 @@ xfs_attr_rmtval_set(
}
/*
+ * Find a hole for the attr and store it in the delayed attr context. This
+ * initializes the context to roll through allocating an attr extent for a
+ * delayed attr operation
+ */
+int
+xfs_attr_rmtval_find_space(
+ struct xfs_delattr_context *dac)
+{
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_bmbt_irec *map = &dac->map;
+ int error;
+
+ dac->lblkno = 0;
+ dac->blkcnt = 0;
+ args->rmtblkcnt = 0;
+ args->rmtblkno = 0;
+ memset(map, 0, sizeof(struct xfs_bmbt_irec));
+
+ error = xfs_attr_rmt_find_hole(args);
+ if (error)
+ return error;
+
+ dac->blkcnt = args->rmtblkcnt;
+ dac->lblkno = args->rmtblkno;
+
+ return 0;
+}
+
+/*
+ * Write one block of the value associated with an attribute into the
+ * out-of-line buffer that we have defined for it. This is similar to a subset
+ * of xfs_attr_rmtval_set, but records the current block to the delayed attr
+ * context, and leaves transaction handling to the caller.
+ */
+int
+xfs_attr_rmtval_set_blk(
+ struct xfs_delattr_context *dac)
+{
+ struct xfs_da_args *args = dac->da_args;
+ struct xfs_inode *dp = args->dp;
+ struct xfs_bmbt_irec *map = &dac->map;
+ int nmap;
+ int error;
+
+ nmap = 1;
+ error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)dac->lblkno,
+ dac->blkcnt, XFS_BMAPI_ATTRFORK, args->total,
+ map, &nmap);
+ if (error)
+ return error;
+
+ ASSERT(nmap == 1);
+ ASSERT((map->br_startblock != DELAYSTARTBLOCK) &&
+ (map->br_startblock != HOLESTARTBLOCK));
+
+ /* roll attribute extent map forwards */
+ dac->lblkno += map->br_blockcount;
+ dac->blkcnt -= map->br_blockcount;
+
+ return 0;
+}
+
+/*
* Remove the value associated with an attribute by deleting the
* out-of-line buffer that it is stored on.
*/
@@ -671,35 +734,6 @@ xfs_attr_rmtval_invalidate(
}
/*
- * Remove the value associated with an attribute by deleting the
- * out-of-line buffer that it is stored on.
- */
-int
-xfs_attr_rmtval_remove(
- struct xfs_da_args *args)
-{
- xfs_dablk_t lblkno;
- int blkcnt;
- int error = 0;
- struct xfs_delattr_context dac = {
- .da_args = args,
- };
-
- trace_xfs_attr_rmtval_remove(args);
-
- /*
- * Keep de-allocating extents until the remote-value region is gone.
- */
- lblkno = args->rmtblkno;
- blkcnt = args->rmtblkcnt;
- do {
- error = __xfs_attr_rmtval_remove(&dac);
- } while (xfs_attr_roll_again(&dac, &error));
-
- return error;
-}
-
-/*
* 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 re-call the function
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 002fd30..84e2700 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -15,4 +15,8 @@ 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_remove(struct xfs_delattr_context *dac);
+int xfs_attr_rmt_find_hole(struct xfs_da_args *args);
+int xfs_attr_rmtval_set_value(struct xfs_da_args *args);
+int xfs_attr_rmtval_set_blk(struct xfs_delattr_context *dac);
+int xfs_attr_rmtval_find_space(struct xfs_delattr_context *dac);
#endif /* __XFS_ATTR_REMOTE_H__ */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 4601366..4c1962d 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1784,7 +1784,6 @@ DEFINE_ATTR_EVENT(xfs_attr_refillstate);
DEFINE_ATTR_EVENT(xfs_attr_rmtval_get);
DEFINE_ATTR_EVENT(xfs_attr_rmtval_set);
-DEFINE_ATTR_EVENT(xfs_attr_rmtval_remove);
#define DEFINE_DA_EVENT(name) \
DEFINE_EVENT(xfs_da_class, name, \
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v10 25/25] xfs: Rename __xfs_attr_rmtval_remove
2020-06-25 23:29 [PATCH v10 00/25] xfs: Delay Ready Attributes Allison Collins
` (23 preceding siblings ...)
2020-06-25 23:30 ` [PATCH v10 24/25] xfs: Add delay ready attr set routines Allison Collins
@ 2020-06-25 23:30 ` Allison Collins
24 siblings, 0 replies; 37+ messages in thread
From: Allison Collins @ 2020-06-25 23:30 UTC (permalink / raw)
To: linux-xfs
Now that xfs_attr_rmtval_remove is gone, rename __xfs_attr_rmtval_remove
to xfs_attr_rmtval_remove
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 7 +++----
fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
fs/xfs/libxfs/xfs_attr_remote.h | 3 +--
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 2d200e3..37c7a28 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -868,7 +868,7 @@ xfs_attr_leaf_addname(
return error;
das_rm_lblk:
if (args->rmtblkno) {
- error = __xfs_attr_rmtval_remove(dac);
+ error = xfs_attr_rmtval_remove(dac);
if (error == -EAGAIN)
dac->dela_state = XFS_DAS_RM_LBLK;
if (error)
@@ -1233,8 +1233,7 @@ xfs_attr_node_addname(
das_rm_nblk:
if (args->rmtblkno) {
- error = __xfs_attr_rmtval_remove(dac);
-
+ error = xfs_attr_rmtval_remove(dac);
if (error == -EAGAIN) {
dac->dela_state = XFS_DAS_RM_NBLK;
return -EAGAIN;
@@ -1392,7 +1391,7 @@ xfs_attr_node_remove_rmt (
/*
* May return -EAGAIN to request that the caller recall this function
*/
- error = __xfs_attr_rmtval_remove(dac);
+ error = xfs_attr_rmtval_remove(dac);
if (error)
return error;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index ceaefb3..6c48d2e 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -739,7 +739,7 @@ xfs_attr_rmtval_invalidate(
* transaction and re-call the function
*/
int
-__xfs_attr_rmtval_remove(
+xfs_attr_rmtval_remove(
struct xfs_delattr_context *dac)
{
struct xfs_da_args *args = dac->da_args;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 84e2700..6ae91af 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -10,11 +10,10 @@ int xfs_attr3_rmt_blocks(struct xfs_mount *mp, int attrlen);
int xfs_attr_rmtval_get(struct xfs_da_args *args);
int xfs_attr_rmtval_set(struct xfs_da_args *args);
-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_remove(struct xfs_delattr_context *dac);
+int xfs_attr_rmtval_remove(struct xfs_delattr_context *dac);
int xfs_attr_rmt_find_hole(struct xfs_da_args *args);
int xfs_attr_rmtval_set_value(struct xfs_da_args *args);
int xfs_attr_rmtval_set_blk(struct xfs_delattr_context *dac);
--
2.7.4
^ permalink raw reply related [flat|nested] 37+ messages in thread