| Message ID | 20210218165348.4754-7-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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 205ad26..bee8d3fb 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_work(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); > @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( > return error; > } > > + error = xfs_attr_node_addname_work(args); > +out: > + if (state) > + xfs_da_state_free(state); > + if (error) > + return error; > + return retval; > +} > + > + > +STATIC > +int xfs_attr_node_addname_work( > + 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. > -- > 2.7.4 >
On 2/24/21 8:04 AM, Brian Foster wrote: > On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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> Alrighty, thanks! Allison > >> 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 205ad26..bee8d3fb 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_work(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); >> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( >> return error; >> } >> >> + error = xfs_attr_node_addname_work(args); >> +out: >> + if (state) >> + xfs_da_state_free(state); >> + if (error) >> + return error; >> + return retval; >> +} >> + >> + >> +STATIC >> +int xfs_attr_node_addname_work( >> + 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. >> -- >> 2.7.4 >> >
On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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> > --- > 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 205ad26..bee8d3fb 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_work(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); > @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( > return error; > } > > + error = xfs_attr_node_addname_work(args); > +out: > + if (state) > + xfs_da_state_free(state); > + if (error) > + return error; > + return retval; > +} > + > + > +STATIC > +int xfs_attr_node_addname_work( What, erm, work does this function do? Since it survives to the end of the patchset, I think this needs a better name (or at least needs a comment about what it's actually supposed to do). AFAICT you're splitting node_addname() into two functions because we're at a transaction roll point, and this "_work" function exists to remove the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old one), right? --D > + 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. > -- > 2.7.4 >
On 2/25/21 9:02 PM, Darrick J. Wong wrote: > On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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> >> --- >> 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 205ad26..bee8d3fb 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_work(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); >> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( >> return error; >> } >> >> + error = xfs_attr_node_addname_work(args); >> +out: >> + if (state) >> + xfs_da_state_free(state); >> + if (error) >> + return error; >> + return retval; >> +} >> + >> + >> +STATIC >> +int xfs_attr_node_addname_work( > > What, erm, work does this function do? Since it survives to the end of > the patchset, I think this needs a better name (or at least needs a > comment about what it's actually supposed to do). To directly answer the question: it's here to help xfs_attr_set_iter not be any bigger than it has to. I think we likely struggled with the name because it's almost like it's just the "remainder" of the operation that doesnt need state management > > AFAICT you're splitting node_addname() into two functions because we're > at a transaction roll point, and this "_work" function exists to remove > the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old > one), right? Thats about right. Maybe just a quick comment? /* * Removes the old xattr key marked with the INCOMPLETE bit */ I suppose we could consider something like "xfs_attr_node_addname_remv_incomplete"? Or xfs_attr_node_addname_cleanup? Trying to cram it into the name maybe getting a bit wordy too. Allison > > --D > >> + 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. >> -- >> 2.7.4 >>
On Fri, Feb 26, 2021 at 05:54:51PM -0700, Allison Henderson wrote: > > > On 2/25/21 9:02 PM, Darrick J. Wong wrote: > > On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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> > > > --- > > > 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 205ad26..bee8d3fb 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_work(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); > > > @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( > > > return error; > > > } > > > + error = xfs_attr_node_addname_work(args); > > > +out: > > > + if (state) > > > + xfs_da_state_free(state); > > > + if (error) > > > + return error; > > > + return retval; > > > +} > > > + > > > + > > > +STATIC > > > +int xfs_attr_node_addname_work( > > > > What, erm, work does this function do? Since it survives to the end of > > the patchset, I think this needs a better name (or at least needs a > > comment about what it's actually supposed to do). > To directly answer the question: it's here to help xfs_attr_set_iter not be > any bigger than it has to. I think we likely struggled with the name because > it's almost like it's just the "remainder" of the operation that doesnt need > state management > > > > > AFAICT you're splitting node_addname() into two functions because we're > > at a transaction roll point, and this "_work" function exists to remove > > the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old > > one), right? > Thats about right. Maybe just a quick comment? > /* > * Removes the old xattr key marked with the INCOMPLETE bit > */ > > I suppose we could consider something like > "xfs_attr_node_addname_remv_incomplete"? Or xfs_attr_node_addname_cleanup? > Trying to cram it into the name maybe getting a bit wordy too. xfs_attr_node_addname_clear_incomplete? --D > > Allison > > > > --D > > > > > + 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. > > > -- > > > 2.7.4 > > >
On 3/1/21 11:00 AM, Darrick J. Wong wrote: > On Fri, Feb 26, 2021 at 05:54:51PM -0700, Allison Henderson wrote: >> >> >> On 2/25/21 9:02 PM, Darrick J. Wong wrote: >>> On Thu, Feb 18, 2021 at 09:53:32AM -0700, 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> >>>> --- >>>> 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 205ad26..bee8d3fb 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_work(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); >>>> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( >>>> return error; >>>> } >>>> + error = xfs_attr_node_addname_work(args); >>>> +out: >>>> + if (state) >>>> + xfs_da_state_free(state); >>>> + if (error) >>>> + return error; >>>> + return retval; >>>> +} >>>> + >>>> + >>>> +STATIC >>>> +int xfs_attr_node_addname_work( >>> >>> What, erm, work does this function do? Since it survives to the end of >>> the patchset, I think this needs a better name (or at least needs a >>> comment about what it's actually supposed to do). >> To directly answer the question: it's here to help xfs_attr_set_iter not be >> any bigger than it has to. I think we likely struggled with the name because >> it's almost like it's just the "remainder" of the operation that doesnt need >> state management >> >>> >>> AFAICT you're splitting node_addname() into two functions because we're >>> at a transaction roll point, and this "_work" function exists to remove >>> the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old >>> one), right? >> Thats about right. Maybe just a quick comment? >> /* >> * Removes the old xattr key marked with the INCOMPLETE bit >> */ >> >> I suppose we could consider something like >> "xfs_attr_node_addname_remv_incomplete"? Or xfs_attr_node_addname_cleanup? >> Trying to cram it into the name maybe getting a bit wordy too. > > xfs_attr_node_addname_clear_incomplete? I'm fine with that as long as everyone else is :-) Allison > > --D > >> >> Allison >>> >>> --D >>> >>>> + 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. >>>> -- >>>> 2.7.4 >>>>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 205ad26..bee8d3fb 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_work(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); @@ -1059,6 +1060,25 @@ xfs_attr_node_addname( return error; } + error = xfs_attr_node_addname_work(args); +out: + if (state) + xfs_da_state_free(state); + if (error) + return error; + return retval; +} + + +STATIC +int xfs_attr_node_addname_work( + 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.
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> --- fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)