| Message ID | 20191107012801.22863-16-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote: > 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> > --- > fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 5dcb19f..626d4a98 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -458,6 +458,27 @@ xfs_attr_set( > goto out_trans_cancel; > > xfs_trans_ijoin(args.trans, dp, 0); > + > + error = xfs_has_attr(&args); > + if (error == -EEXIST) { > + if (name->type & ATTR_CREATE) > + goto out_trans_cancel; > + else > + name->type |= ATTR_REPLACE; > + } > + > + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) > + goto out_trans_cancel; > + > + if (name->type & ATTR_REPLACE) { > + name->type &= ~ATTR_REPLACE; > + error = xfs_attr_remove_args(&args); > + if (error) > + goto out_trans_cancel; > + > + name->type |= ATTR_CREATE; I thought _set_args already handled the remove part of replacing an attr? And I thought that it did this with an atomic rename? Won't this break the atomicity of attr replacement? --D > + } > + > error = xfs_attr_set_args(&args); > if (error) > goto out_trans_cancel; > @@ -543,6 +564,10 @@ xfs_attr_remove( > */ > xfs_trans_ijoin(args.trans, dp, 0); > > + error = xfs_has_attr(&args); > + if (error == -ENOATTR) > + goto out; > + > error = xfs_attr_remove_args(&args); > if (error) > goto out; > -- > 2.7.4 >
On 11/8/19 2:28 PM, Darrick J. Wong wrote: > On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote: >> 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> >> --- >> fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 5dcb19f..626d4a98 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -458,6 +458,27 @@ xfs_attr_set( >> goto out_trans_cancel; >> >> xfs_trans_ijoin(args.trans, dp, 0); >> + >> + error = xfs_has_attr(&args); >> + if (error == -EEXIST) { >> + if (name->type & ATTR_CREATE) >> + goto out_trans_cancel; >> + else >> + name->type |= ATTR_REPLACE; >> + } >> + >> + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) >> + goto out_trans_cancel; >> + >> + if (name->type & ATTR_REPLACE) { >> + name->type &= ~ATTR_REPLACE; >> + error = xfs_attr_remove_args(&args); >> + if (error) >> + goto out_trans_cancel; >> + >> + name->type |= ATTR_CREATE; > > I thought _set_args already handled the remove part of replacing an > attr? No, IIRC in one of the other reviews we decided to break the rename into a set and then a remove. That way the error handling moves up here instead of trying to deal with it in the middle of the delayed operation And I thought that it did this with an atomic rename? Won't this > break the atomicity of attr replacement? Hmm, think this worked for delayed operations, but not anymore since we're going back to supporting both delayed and inline in one code path. I think what this means is that the flip flag has to get moved in here, right? We flip on the incomplete flag before the remove and then set it when the rename is done? > > --D > >> + } >> + >> error = xfs_attr_set_args(&args); >> if (error) >> goto out_trans_cancel; >> @@ -543,6 +564,10 @@ xfs_attr_remove( >> */ >> xfs_trans_ijoin(args.trans, dp, 0); >> >> + error = xfs_has_attr(&args); >> + if (error == -ENOATTR) >> + goto out; >> + >> error = xfs_attr_remove_args(&args); >> if (error) >> goto out; >> -- >> 2.7.4 >>
On Fri, Nov 08, 2019 at 02:42:52PM -0700, Allison Collins wrote: > On 11/8/19 2:28 PM, Darrick J. Wong wrote: > > On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote: > > > 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> > > > --- > > > fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > index 5dcb19f..626d4a98 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > @@ -458,6 +458,27 @@ xfs_attr_set( > > > goto out_trans_cancel; > > > xfs_trans_ijoin(args.trans, dp, 0); > > > + > > > + error = xfs_has_attr(&args); > > > + if (error == -EEXIST) { > > > + if (name->type & ATTR_CREATE) > > > + goto out_trans_cancel; > > > + else > > > + name->type |= ATTR_REPLACE; > > > + } > > > + > > > + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) > > > + goto out_trans_cancel; > > > + > > > + if (name->type & ATTR_REPLACE) { > > > + name->type &= ~ATTR_REPLACE; > > > + error = xfs_attr_remove_args(&args); > > > + if (error) > > > + goto out_trans_cancel; > > > + > > > + name->type |= ATTR_CREATE; > > > > I thought _set_args already handled the remove part of replacing an > > attr? > No, IIRC in one of the other reviews we decided to break the rename into a > set and then a remove. But this looks like we remove the old attr before setting the new one, which means that if we crash right here we'll come back up with no attr at all. The INCOMPLETE flag flipping trick only works for ATTR_REPLACE if you add the new attr before removing the old one. (Or am I misreading something here?) > That way the error handling moves up here instead of > trying to deal with it in the middle of the delayed operation Sounds good. > > And I thought that it did this with an atomic rename? Won't this > > break the atomicity of attr replacement? > Hmm, think this worked for delayed operations, but not anymore since we're > going back to supporting both delayed and inline in one code path. I think > what this means is that the flip flag has to get moved in here, right? We > flip on the incomplete flag before the remove and then set it when the > rename is done? Yeah. --D > > > > > --D > > > > > + } > > > + > > > error = xfs_attr_set_args(&args); > > > if (error) > > > goto out_trans_cancel; > > > @@ -543,6 +564,10 @@ xfs_attr_remove( > > > */ > > > xfs_trans_ijoin(args.trans, dp, 0); > > > + error = xfs_has_attr(&args); > > > + if (error == -ENOATTR) > > > + goto out; > > > + > > > error = xfs_attr_remove_args(&args); > > > if (error) > > > goto out; > > > -- > > > 2.7.4 > > >
On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote: > 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> > --- > fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 5dcb19f..626d4a98 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -458,6 +458,27 @@ xfs_attr_set( > goto out_trans_cancel; > > xfs_trans_ijoin(args.trans, dp, 0); > + > + error = xfs_has_attr(&args); > + if (error == -EEXIST) { > + if (name->type & ATTR_CREATE) > + goto out_trans_cancel; > + else > + name->type |= ATTR_REPLACE; > + } > + > + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) > + goto out_trans_cancel; > + > + if (name->type & ATTR_REPLACE) { > + name->type &= ~ATTR_REPLACE; > + error = xfs_attr_remove_args(&args); > + if (error) > + goto out_trans_cancel; > + > + name->type |= ATTR_CREATE; > + } > + I see Darrick already commented on this.. I think the behavior of the existing rename code is to essentially create the new xattr with the INCOMPLETE flag set so we can roll transactions, etc. without any observable behavior to userspace. Once the new xattr is fully in place, the rename is performed atomically from the userspace perspective by flipping the INCOMPLETE flag from the newly constructed xattr to the old one and we can then remove the old xattr from there. > error = xfs_attr_set_args(&args); > if (error) > goto out_trans_cancel; > @@ -543,6 +564,10 @@ xfs_attr_remove( > */ > xfs_trans_ijoin(args.trans, dp, 0); > > + error = xfs_has_attr(&args); > + if (error == -ENOATTR) > + goto out; > + Wouldn't we want to return any error that might occur here (except -EEXIST), not just -ENOATTR if there's actually no xattr? Brian > error = xfs_attr_remove_args(&args); > if (error) > goto out; > -- > 2.7.4 >
On 11/11/19 11:24 AM, Brian Foster wrote: > On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote: >> 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> >> --- >> fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 5dcb19f..626d4a98 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -458,6 +458,27 @@ xfs_attr_set( >> goto out_trans_cancel; >> >> xfs_trans_ijoin(args.trans, dp, 0); >> + >> + error = xfs_has_attr(&args); >> + if (error == -EEXIST) { >> + if (name->type & ATTR_CREATE) >> + goto out_trans_cancel; >> + else >> + name->type |= ATTR_REPLACE; >> + } >> + >> + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) >> + goto out_trans_cancel; >> + >> + if (name->type & ATTR_REPLACE) { >> + name->type &= ~ATTR_REPLACE; >> + error = xfs_attr_remove_args(&args); >> + if (error) >> + goto out_trans_cancel; >> + >> + name->type |= ATTR_CREATE; >> + } >> + > > I see Darrick already commented on this.. I think the behavior of the > existing rename code is to essentially create the new xattr with the > INCOMPLETE flag set so we can roll transactions, etc. without any > observable behavior to userspace. Once the new xattr is fully in place, > the rename is performed atomically from the userspace perspective by > flipping the INCOMPLETE flag from the newly constructed xattr to the old > one and we can then remove the old xattr from there. Yes, I will add this logic in the next revision > >> error = xfs_attr_set_args(&args); >> if (error) >> goto out_trans_cancel; >> @@ -543,6 +564,10 @@ xfs_attr_remove( >> */ >> xfs_trans_ijoin(args.trans, dp, 0); >> >> + error = xfs_has_attr(&args); >> + if (error == -ENOATTR) >> + goto out; >> + > > Wouldn't we want to return any error that might occur here (except > -EEXIST), not just -ENOATTR if there's actually no xattr? > > Brian Ok, I will change this to (error != -EEXIST) Thanks for the reviews! Allison > >> error = xfs_attr_remove_args(&args); >> if (error) >> goto out; >> -- >> 2.7.4 >> >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 5dcb19f..626d4a98 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -458,6 +458,27 @@ xfs_attr_set( goto out_trans_cancel; xfs_trans_ijoin(args.trans, dp, 0); + + error = xfs_has_attr(&args); + if (error == -EEXIST) { + if (name->type & ATTR_CREATE) + goto out_trans_cancel; + else + name->type |= ATTR_REPLACE; + } + + if (error == -ENOATTR && (name->type & ATTR_REPLACE)) + goto out_trans_cancel; + + if (name->type & ATTR_REPLACE) { + name->type &= ~ATTR_REPLACE; + error = xfs_attr_remove_args(&args); + if (error) + goto out_trans_cancel; + + name->type |= ATTR_CREATE; + } + error = xfs_attr_set_args(&args); if (error) goto out_trans_cancel; @@ -543,6 +564,10 @@ xfs_attr_remove( */ xfs_trans_ijoin(args.trans, dp, 0); + error = xfs_has_attr(&args); + if (error == -ENOATTR) + goto out; + error = xfs_attr_remove_args(&args); if (error) goto out;
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> --- fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)