| Message ID | 20210218165348.4754-11-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Thu, Feb 18, 2021 at 09:53:36AM -0700, Allison Henderson wrote: > This patch basically hoists the node transaction handling around the > leaf code we just hoisted. This will helps setup this area for the > state machine since the goto is easily replaced with a state since it > ends with a transaction roll. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++----------------------- > 1 file changed, 28 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index bfd4466..56d4b56 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -288,8 +288,34 @@ xfs_attr_set_args( > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > error = xfs_attr_leaf_try_add(args, bp); > - if (error == -ENOSPC) > + if (error == -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) > + 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; > + > goto node; > + } > else if (error) } else if (error) { return error; } (I think we usually try to add braces around all branches of an if/else if at least one branch requires them.) Otherwise, the factoring looks Ok to me and this does improve on the wart from the previous patch: Reviewed-by: Brian Foster <bfoster@redhat.com> > return error; > > @@ -381,32 +407,9 @@ xfs_attr_set_args( > /* bp is gone due to xfs_da_shrink_inode */ > > return error; > + } > node: > - /* > - * 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) > - 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; > - } > > do { > error = xfs_attr_node_addname_find_attr(args, &state); > -- > 2.7.4 >
On 2/24/21 11:43 AM, Brian Foster wrote: > On Thu, Feb 18, 2021 at 09:53:36AM -0700, Allison Henderson wrote: >> This patch basically hoists the node transaction handling around the >> leaf code we just hoisted. This will helps setup this area for the >> state machine since the goto is easily replaced with a state since it >> ends with a transaction roll. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++----------------------- >> 1 file changed, 28 insertions(+), 25 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index bfd4466..56d4b56 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -288,8 +288,34 @@ xfs_attr_set_args( >> >> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> error = xfs_attr_leaf_try_add(args, bp); >> - if (error == -ENOSPC) >> + if (error == -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) >> + 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; >> + >> goto node; >> + } >> else if (error) > > } else if (error) { > return error; > } > > (I think we usually try to add braces around all branches of an if/else > if at least one branch requires them.) Ok, will fix > > Otherwise, the factoring looks Ok to me and this does improve on the > wart from the previous patch: > > Reviewed-by: Brian Foster <bfoster@redhat.com> Great, thanks! Allison > >> return error; >> >> @@ -381,32 +407,9 @@ xfs_attr_set_args( >> /* bp is gone due to xfs_da_shrink_inode */ >> >> return error; >> + } >> node: >> - /* >> - * 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) >> - 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; >> - } >> >> do { >> error = xfs_attr_node_addname_find_attr(args, &state); >> -- >> 2.7.4 >> >
On Thu, Feb 18, 2021 at 09:53:36AM -0700, Allison Henderson wrote: > This patch basically hoists the node transaction handling around the > leaf code we just hoisted. This will helps setup this area for the > state machine since the goto is easily replaced with a state since it > ends with a transaction roll. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++----------------------- > 1 file changed, 28 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index bfd4466..56d4b56 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -288,8 +288,34 @@ xfs_attr_set_args( > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > error = xfs_attr_leaf_try_add(args, bp); > - if (error == -ENOSPC) > + if (error == -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) > + 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; > + > goto node; > + } > else if (error) > return error; With the braces and indenting fixed the way Brian said, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > @@ -381,32 +407,9 @@ xfs_attr_set_args( > /* bp is gone due to xfs_da_shrink_inode */ > > return error; > + } > node: > - /* > - * 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) > - 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; > - } > > do { > error = xfs_attr_node_addname_find_attr(args, &state); > -- > 2.7.4 >
On 3/1/21 11:20 AM, Darrick J. Wong wrote: > On Thu, Feb 18, 2021 at 09:53:36AM -0700, Allison Henderson wrote: >> This patch basically hoists the node transaction handling around the >> leaf code we just hoisted. This will helps setup this area for the >> state machine since the goto is easily replaced with a state since it >> ends with a transaction roll. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++----------------------- >> 1 file changed, 28 insertions(+), 25 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index bfd4466..56d4b56 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -288,8 +288,34 @@ xfs_attr_set_args( >> >> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> error = xfs_attr_leaf_try_add(args, bp); >> - if (error == -ENOSPC) >> + if (error == -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) >> + 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; >> + >> goto node; >> + } >> else if (error) >> return error; > > With the braces and indenting fixed the way Brian said, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Sure, will do. Thank you! Allison > > --D > >> >> @@ -381,32 +407,9 @@ xfs_attr_set_args( >> /* bp is gone due to xfs_da_shrink_inode */ >> >> return error; >> + } >> node: >> - /* >> - * 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) >> - 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; >> - } >> >> do { >> error = xfs_attr_node_addname_find_attr(args, &state); >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index bfd4466..56d4b56 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -288,8 +288,34 @@ xfs_attr_set_args( if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { error = xfs_attr_leaf_try_add(args, bp); - if (error == -ENOSPC) + if (error == -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) + 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; + goto node; + } else if (error) return error; @@ -381,32 +407,9 @@ xfs_attr_set_args( /* bp is gone due to xfs_da_shrink_inode */ return error; + } node: - /* - * 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) - 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; - } do { error = xfs_attr_node_addname_find_attr(args, &state);
This patch basically hoists the node transaction handling around the leaf code we just hoisted. This will helps setup this area for the state machine since the goto is easily replaced with a state since it ends with a transaction roll. Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 53 +++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 25 deletions(-)