| Message ID | 20210416092045.2215-6-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On 16 Apr 2021 at 14:50, Allison Henderson wrote: > This patch separate xfs_attr_node_addname into two functions. This will > help to make it easier to hoist parts of xfs_attr_node_addname that need > state management > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index fff1c6f..d9dfc8d2 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -54,6 +54,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_addname_clear_incomplete(struct xfs_da_args *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); > @@ -1062,6 +1063,25 @@ xfs_attr_node_addname( > return error; > } > > + error = xfs_attr_node_addname_clear_incomplete(args); > +out: > + if (state) > + xfs_da_state_free(state); > + if (error) > + return error; > + return retval; I think the above code incorrectly returns -ENOSPC when the user is performing an xattr rename operation and the call to xfs_attr3_leaf_add() resulted in returning -ENOSPC, 1. xfs_attr3_leaf_add() returns -ENOSPC. 2. xfs_da3_split() allocates a new leaf and inserts the new xattr into it. 3. If the user was performing a rename operation (i.e. XFS_DA_OP_RENAME is set), we flip the "incomplete" flag. 4. Remove the old xattr's remote blocks (if any). 5. Remove old xattr's name. 6. If "error" has zero as its value, we return the value of "retval". At this point in execution, "retval" would have -ENOSPC as its value. -- chandan
On 4/18/21 10:15 PM, Chandan Babu R wrote: > On 16 Apr 2021 at 14:50, Allison Henderson wrote: >> This patch separate xfs_attr_node_addname into two functions. This will >> help to make it easier to hoist parts of xfs_attr_node_addname that need >> state management >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> Reviewed-by: Brian Foster <bfoster@redhat.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index fff1c6f..d9dfc8d2 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -54,6 +54,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_addname_clear_incomplete(struct xfs_da_args *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); >> @@ -1062,6 +1063,25 @@ xfs_attr_node_addname( >> return error; >> } >> >> + error = xfs_attr_node_addname_clear_incomplete(args); Ok, so i think what we need here is an extra few lines below. That's similar to the original exit handling if (error) goto out; retval = error = 0; Looks good? >> +out: >> + if (state) >> + xfs_da_state_free(state); >> + if (error) >> + return error; >> + return retval; > > I think the above code incorrectly returns -ENOSPC when the user is performing > an xattr rename operation and the call to xfs_attr3_leaf_add() resulted in > returning -ENOSPC, > 1. xfs_attr3_leaf_add() returns -ENOSPC. > 2. xfs_da3_split() allocates a new leaf and inserts the new xattr into it. > 3. If the user was performing a rename operation (i.e. XFS_DA_OP_RENAME is > set), we flip the "incomplete" flag. > 4. Remove the old xattr's remote blocks (if any). > 5. Remove old xattr's name. > 6. If "error" has zero as its value, we return the value of "retval". At this > point in execution, "retval" would have -ENOSPC as its value. > > -- > chandan Thanks for the catch! Allison >
On 20 Apr 2021 at 00:02, Allison Henderson wrote: > On 4/18/21 10:15 PM, Chandan Babu R wrote: >> On 16 Apr 2021 at 14:50, Allison Henderson wrote: >>> This patch separate xfs_attr_node_addname into two functions. This will >>> help to make it easier to hoist parts of xfs_attr_node_addname that need >>> state management >>> >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>> Reviewed-by: Brian Foster <bfoster@redhat.com> >>> --- >>> fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>> index fff1c6f..d9dfc8d2 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.c >>> +++ b/fs/xfs/libxfs/xfs_attr.c >>> @@ -54,6 +54,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_addname_clear_incomplete(struct xfs_da_args *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); >>> @@ -1062,6 +1063,25 @@ xfs_attr_node_addname( >>> return error; >>> } >>> >>> + error = xfs_attr_node_addname_clear_incomplete(args); > Ok, so i think what we need here is an extra few lines below. That's > similar to the original exit handling > > if (error) > goto out; > retval = error = 0; "retval = 0;" should suffice. > > Looks good? > >>> +out: >>> + if (state) >>> + xfs_da_state_free(state); >>> + if (error) >>> + return error; >>> + return retval; >> >> I think the above code incorrectly returns -ENOSPC when the user is performing >> an xattr rename operation and the call to xfs_attr3_leaf_add() resulted in >> returning -ENOSPC, >> 1. xfs_attr3_leaf_add() returns -ENOSPC. >> 2. xfs_da3_split() allocates a new leaf and inserts the new xattr into it. >> 3. If the user was performing a rename operation (i.e. XFS_DA_OP_RENAME is >> set), we flip the "incomplete" flag. >> 4. Remove the old xattr's remote blocks (if any). >> 5. Remove old xattr's name. >> 6. If "error" has zero as its value, we return the value of "retval". At this >> point in execution, "retval" would have -ENOSPC as its value. >> > Thanks for the catch! > Allison
On 4/20/21 5:14 AM, Chandan Babu R wrote: > On 20 Apr 2021 at 00:02, Allison Henderson wrote: >> On 4/18/21 10:15 PM, Chandan Babu R wrote: >>> On 16 Apr 2021 at 14:50, Allison Henderson wrote: >>>> This patch separate xfs_attr_node_addname into two functions. This will >>>> help to make it easier to hoist parts of xfs_attr_node_addname that need >>>> state management >>>> >>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>>> Reviewed-by: Brian Foster <bfoster@redhat.com> >>>> --- >>>> fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>>> index fff1c6f..d9dfc8d2 100644 >>>> --- a/fs/xfs/libxfs/xfs_attr.c >>>> +++ b/fs/xfs/libxfs/xfs_attr.c >>>> @@ -54,6 +54,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_addname_clear_incomplete(struct xfs_da_args *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); >>>> @@ -1062,6 +1063,25 @@ xfs_attr_node_addname( >>>> return error; >>>> } >>>> >>>> + error = xfs_attr_node_addname_clear_incomplete(args); >> Ok, so i think what we need here is an extra few lines below. That's >> similar to the original exit handling >> >> if (error) >> goto out; >> retval = error = 0; > > "retval = 0;" should suffice. Ok, will that add in. Thanks! Allison > >> >> Looks good? >> >>>> +out: >>>> + if (state) >>>> + xfs_da_state_free(state); >>>> + if (error) >>>> + return error; >>>> + return retval; >>> >>> I think the above code incorrectly returns -ENOSPC when the user is performing >>> an xattr rename operation and the call to xfs_attr3_leaf_add() resulted in >>> returning -ENOSPC, >>> 1. xfs_attr3_leaf_add() returns -ENOSPC. >>> 2. xfs_da3_split() allocates a new leaf and inserts the new xattr into it. >>> 3. If the user was performing a rename operation (i.e. XFS_DA_OP_RENAME is >>> set), we flip the "incomplete" flag. >>> 4. Remove the old xattr's remote blocks (if any). >>> 5. Remove old xattr's name. >>> 6. If "error" has zero as its value, we return the value of "retval". At this >>> point in execution, "retval" would have -ENOSPC as its value. >>> >> Thanks for the catch! >> Allison >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index fff1c6f..d9dfc8d2 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -54,6 +54,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_addname_clear_incomplete(struct xfs_da_args *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); @@ -1062,6 +1063,25 @@ xfs_attr_node_addname( return error; } + error = xfs_attr_node_addname_clear_incomplete(args); +out: + if (state) + xfs_da_state_free(state); + if (error) + return error; + return retval; +} + + +STATIC +int xfs_attr_node_addname_clear_incomplete( + struct xfs_da_args *args) +{ + struct xfs_da_state *state = NULL; + struct xfs_da_state_blk *blk; + int retval = 0; + int error = 0; + /* * 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.