| Message ID | 20191107012801.22863-12-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:55PM -0700, Allison Collins wrote: > And new helper functions xfs_attr3_leaf_flag_is_set and > xfs_attr3_leaf_flagsflipped. These routines check to see > if xfs_attr3_leaf_setflag or xfs_attr3_leaf_flipflags have > already been run. We will need this later for delayed > attributes since routines may be recalled several times > when -EAGAIN is returned. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr_leaf.c | 94 +++++++++++++++++++++++++++++++++++++++++++ > fs/xfs/libxfs/xfs_attr_leaf.h | 2 + > 2 files changed, 96 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 42c037e..023c616 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -2809,6 +2809,40 @@ xfs_attr3_leaf_clearflag( > } > > /* > + * Check if the INCOMPLETE flag on an entry in a leaf block is set. This > + * function can be used to check if xfs_attr3_leaf_setflag has already been > + * called. The INCOMPLETE flag is used during attr rename operations to mark > + * entries that are being renamed. Since renames should be atomic, only one of It's also used when creating an xattr with a value stored in a remote block so that we can commit the name entry to the log (with INCOMPLETE set), allocate/write the remote value with ordered buffers, and then commit a second transaction clearing the INCOMPLETE flag. Now that I think about it ... this predicate is for non-rename setting of attrs with remote values, and the "flagsflipped" predicate is for rename operations, aren't they? > + * them should appear as a completed attribute. > + * > + * isset is set to true if the flag is set or false otherwise > + */ > +int > +xfs_attr3_leaf_flag_is_set( > + struct xfs_da_args *args, > + bool *isset) > +{ > + struct xfs_attr_leafblock *leaf; > + struct xfs_attr_leaf_entry *entry; > + struct xfs_buf *bp; > + struct xfs_inode *dp = args->dp; > + int error = 0; > + > + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, > + XFS_DABUF_MAP_NOMAPPING, &bp); > + if (error) > + return error; > + > + leaf = bp->b_addr; > + entry = &xfs_attr3_leaf_entryp(leaf)[args->index]; > + > + *isset = ((entry->flags & XFS_ATTR_INCOMPLETE) != 0); > + xfs_trans_brelse(args->trans, bp); > + > + return 0; > +} > + > +/* > * Set the INCOMPLETE flag on an entry in a leaf block. > */ > int > @@ -2972,3 +3006,63 @@ xfs_attr3_leaf_flipflags( > > return error; > } > + > +/* > + * On a leaf entry, check to see if the INCOMPLETE flag is cleared > + * in args->blkno/index and set in args->blkno2/index2. Might be worth mentioning here that args->blkno is the old entry and args->blkno2 is the new entry. This predicate will be used (by the deferred attr item recovery code) to decide if we have to finish that part of a rename operation, right? > Note that they could be > + * in different blocks, or in the same block. This function can be used to > + * check if xfs_attr3_leaf_flipflags has already been called. The INCOMPLETE > + * flag is used during attr rename operations to mark entries that are being > + * renamed. Since renames should be atomic, only one of them should appear as a > + * completed attribute. > + * > + * isflipped is set to true if flags are flipped or false otherwise > + */ > +int > +xfs_attr3_leaf_flagsflipped( I don't like "flagsflipped" because it's not clear to me what "flipped" means. xfs_attr3_leaf_rename_is_incomplete() ? > + struct xfs_da_args *args, > + bool *isflipped) > +{ > + struct xfs_attr_leafblock *leaf1; > + struct xfs_attr_leafblock *leaf2; > + struct xfs_attr_leaf_entry *entry1; > + struct xfs_attr_leaf_entry *entry2; > + struct xfs_buf *bp1; > + struct xfs_buf *bp2; > + struct xfs_inode *dp = args->dp; > + int error = 0; > + > + /* > + * Read the block containing the "old" attr > + */ > + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, > + XFS_DABUF_MAP_NOMAPPING, &bp1); > + if (error) > + return error; > + > + /* > + * Read the block containing the "new" attr, if it is different > + */ > + if (args->blkno2 != args->blkno) { > + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno2, > + -1, &bp2); > + if (error) bp1 leaks here, I think. > + return error; > + } else { > + bp2 = bp1; > + } > + > + leaf1 = bp1->b_addr; > + entry1 = &xfs_attr3_leaf_entryp(leaf1)[args->index]; > + > + leaf2 = bp2->b_addr; > + entry2 = &xfs_attr3_leaf_entryp(leaf2)[args->index2]; > + > + *isflipped = (((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) && Nit: ((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) could be written as !(entry1->flags & XFS_ATTR_INCOMPLETE) > + (entry2->flags & XFS_ATTR_INCOMPLETE)); > + > + xfs_trans_brelse(args->trans, bp1); > + xfs_trans_brelse(args->trans, bp2); This double-frees bp2 if bp1 == bp2. --D > + > + return 0; > +} > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h > index e108b37..12283cf 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.h > +++ b/fs/xfs/libxfs/xfs_attr_leaf.h > @@ -57,7 +57,9 @@ int xfs_attr3_leaf_to_shortform(struct xfs_buf *bp, > struct xfs_da_args *args, int forkoff); > int xfs_attr3_leaf_clearflag(struct xfs_da_args *args); > int xfs_attr3_leaf_setflag(struct xfs_da_args *args); > +int xfs_attr3_leaf_flag_is_set(struct xfs_da_args *args, bool *isset); > int xfs_attr3_leaf_flipflags(struct xfs_da_args *args); > +int xfs_attr3_leaf_flagsflipped(struct xfs_da_args *args, bool *isflipped); > > /* > * Routines used for growing the Btree. > -- > 2.7.4 >
On 11/8/19 2:17 PM, Darrick J. Wong wrote: > On Wed, Nov 06, 2019 at 06:27:55PM -0700, Allison Collins wrote: >> And new helper functions xfs_attr3_leaf_flag_is_set and >> xfs_attr3_leaf_flagsflipped. These routines check to see >> if xfs_attr3_leaf_setflag or xfs_attr3_leaf_flipflags have >> already been run. We will need this later for delayed >> attributes since routines may be recalled several times >> when -EAGAIN is returned. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr_leaf.c | 94 +++++++++++++++++++++++++++++++++++++++++++ >> fs/xfs/libxfs/xfs_attr_leaf.h | 2 + >> 2 files changed, 96 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c >> index 42c037e..023c616 100644 >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c >> @@ -2809,6 +2809,40 @@ xfs_attr3_leaf_clearflag( >> } >> >> /* >> + * Check if the INCOMPLETE flag on an entry in a leaf block is set. This >> + * function can be used to check if xfs_attr3_leaf_setflag has already been >> + * called. The INCOMPLETE flag is used during attr rename operations to mark >> + * entries that are being renamed. Since renames should be atomic, only one of > > It's also used when creating an xattr with a value stored in a remote > block so that we can commit the name entry to the log (with INCOMPLETE > set), allocate/write the remote value with ordered buffers, and then > commit a second transaction clearing the INCOMPLETE flag. > > Now that I think about it ... this predicate is for non-rename setting > of attrs with remote values, and the "flagsflipped" predicate is for > rename operations, aren't they?\ Yes, but I'm realizing now, that we may not need these helpers if we continue to move forward with the state machine. With the state machine, we can simply flip the flag, set the next state, and move on since that region of code will not execute again. The state switch will jump us forward to where we were without having to check if we already did it. > >> + * them should appear as a completed attribute. >> + * >> + * isset is set to true if the flag is set or false otherwise >> + */ >> +int >> +xfs_attr3_leaf_flag_is_set( >> + struct xfs_da_args *args, >> + bool *isset) >> +{ >> + struct xfs_attr_leafblock *leaf; >> + struct xfs_attr_leaf_entry *entry; >> + struct xfs_buf *bp; >> + struct xfs_inode *dp = args->dp; >> + int error = 0; >> + >> + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, >> + XFS_DABUF_MAP_NOMAPPING, &bp); >> + if (error) >> + return error; >> + >> + leaf = bp->b_addr; >> + entry = &xfs_attr3_leaf_entryp(leaf)[args->index]; >> + >> + *isset = ((entry->flags & XFS_ATTR_INCOMPLETE) != 0); >> + xfs_trans_brelse(args->trans, bp); >> + >> + return 0; >> +} >> + >> +/* >> * Set the INCOMPLETE flag on an entry in a leaf block. >> */ >> int >> @@ -2972,3 +3006,63 @@ xfs_attr3_leaf_flipflags( >> >> return error; >> } >> + >> +/* >> + * On a leaf entry, check to see if the INCOMPLETE flag is cleared >> + * in args->blkno/index and set in args->blkno2/index2. > > Might be worth mentioning here that args->blkno is the old entry and > args->blkno2 is the new entry. This predicate will be used (by the > deferred attr item recovery code) to decide if we have to finish that > part of a rename operation, right? No, it was used in the previous series xfs_attr_set_later where I was aiming to rely more on the state of the fork rather than the state machine. But with the new state machine in this series, I may be able to just drop this patch. > >> Note that they could be >> + * in different blocks, or in the same block. This function can be used to >> + * check if xfs_attr3_leaf_flipflags has already been called. The INCOMPLETE >> + * flag is used during attr rename operations to mark entries that are being >> + * renamed. Since renames should be atomic, only one of them should appear as a >> + * completed attribute. >> + * >> + * isflipped is set to true if flags are flipped or false otherwise >> + */ >> +int >> +xfs_attr3_leaf_flagsflipped( > > I don't like "flagsflipped" because it's not clear to me what "flipped" > means. > > xfs_attr3_leaf_rename_is_incomplete() ? > Sure, if I keep this patch in the set I will update it. Things are still pretty wiggly, so i may still find a use for it, but hopefully I can let it go. >> + struct xfs_da_args *args, >> + bool *isflipped) >> +{ >> + struct xfs_attr_leafblock *leaf1; >> + struct xfs_attr_leafblock *leaf2; >> + struct xfs_attr_leaf_entry *entry1; >> + struct xfs_attr_leaf_entry *entry2; >> + struct xfs_buf *bp1; >> + struct xfs_buf *bp2; >> + struct xfs_inode *dp = args->dp; >> + int error = 0; >> + >> + /* >> + * Read the block containing the "old" attr >> + */ >> + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, >> + XFS_DABUF_MAP_NOMAPPING, &bp1); >> + if (error) >> + return error; >> + >> + /* >> + * Read the block containing the "new" attr, if it is different >> + */ >> + if (args->blkno2 != args->blkno) { >> + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno2, >> + -1, &bp2); >> + if (error) > > bp1 leaks here, I think. ok, will make a note. > >> + return error; >> + } else { >> + bp2 = bp1; >> + } >> + >> + leaf1 = bp1->b_addr; >> + entry1 = &xfs_attr3_leaf_entryp(leaf1)[args->index]; >> + >> + leaf2 = bp2->b_addr; >> + entry2 = &xfs_attr3_leaf_entryp(leaf2)[args->index2]; >> + >> + *isflipped = (((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) && > > Nit: ((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) could be written as > !(entry1->flags & XFS_ATTR_INCOMPLETE) > >> + (entry2->flags & XFS_ATTR_INCOMPLETE)); >> + >> + xfs_trans_brelse(args->trans, bp1); >> + xfs_trans_brelse(args->trans, bp2); > > This double-frees bp2 if bp1 == bp2. Ok, will fix if needed. Thanks for the review! Sorry for the stray patch! Allison > > --D > > >> + >> + return 0; >> +} >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h >> index e108b37..12283cf 100644 >> --- a/fs/xfs/libxfs/xfs_attr_leaf.h >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.h >> @@ -57,7 +57,9 @@ int xfs_attr3_leaf_to_shortform(struct xfs_buf *bp, >> struct xfs_da_args *args, int forkoff); >> int xfs_attr3_leaf_clearflag(struct xfs_da_args *args); >> int xfs_attr3_leaf_setflag(struct xfs_da_args *args); >> +int xfs_attr3_leaf_flag_is_set(struct xfs_da_args *args, bool *isset); >> int xfs_attr3_leaf_flipflags(struct xfs_da_args *args); >> +int xfs_attr3_leaf_flagsflipped(struct xfs_da_args *args, bool *isflipped); >> >> /* >> * Routines used for growing the Btree. >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 42c037e..023c616 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -2809,6 +2809,40 @@ xfs_attr3_leaf_clearflag( } /* + * Check if the INCOMPLETE flag on an entry in a leaf block is set. This + * function can be used to check if xfs_attr3_leaf_setflag has already been + * called. The INCOMPLETE flag is used during attr rename operations to mark + * entries that are being renamed. Since renames should be atomic, only one of + * them should appear as a completed attribute. + * + * isset is set to true if the flag is set or false otherwise + */ +int +xfs_attr3_leaf_flag_is_set( + struct xfs_da_args *args, + bool *isset) +{ + struct xfs_attr_leafblock *leaf; + struct xfs_attr_leaf_entry *entry; + struct xfs_buf *bp; + struct xfs_inode *dp = args->dp; + int error = 0; + + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, + XFS_DABUF_MAP_NOMAPPING, &bp); + if (error) + return error; + + leaf = bp->b_addr; + entry = &xfs_attr3_leaf_entryp(leaf)[args->index]; + + *isset = ((entry->flags & XFS_ATTR_INCOMPLETE) != 0); + xfs_trans_brelse(args->trans, bp); + + return 0; +} + +/* * Set the INCOMPLETE flag on an entry in a leaf block. */ int @@ -2972,3 +3006,63 @@ xfs_attr3_leaf_flipflags( return error; } + +/* + * On a leaf entry, check to see if the INCOMPLETE flag is cleared + * in args->blkno/index and set in args->blkno2/index2. Note that they could be + * in different blocks, or in the same block. This function can be used to + * check if xfs_attr3_leaf_flipflags has already been called. The INCOMPLETE + * flag is used during attr rename operations to mark entries that are being + * renamed. Since renames should be atomic, only one of them should appear as a + * completed attribute. + * + * isflipped is set to true if flags are flipped or false otherwise + */ +int +xfs_attr3_leaf_flagsflipped( + struct xfs_da_args *args, + bool *isflipped) +{ + struct xfs_attr_leafblock *leaf1; + struct xfs_attr_leafblock *leaf2; + struct xfs_attr_leaf_entry *entry1; + struct xfs_attr_leaf_entry *entry2; + struct xfs_buf *bp1; + struct xfs_buf *bp2; + struct xfs_inode *dp = args->dp; + int error = 0; + + /* + * Read the block containing the "old" attr + */ + error = xfs_attr3_leaf_read(args->trans, dp, args->blkno, + XFS_DABUF_MAP_NOMAPPING, &bp1); + if (error) + return error; + + /* + * Read the block containing the "new" attr, if it is different + */ + if (args->blkno2 != args->blkno) { + error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno2, + -1, &bp2); + if (error) + return error; + } else { + bp2 = bp1; + } + + leaf1 = bp1->b_addr; + entry1 = &xfs_attr3_leaf_entryp(leaf1)[args->index]; + + leaf2 = bp2->b_addr; + entry2 = &xfs_attr3_leaf_entryp(leaf2)[args->index2]; + + *isflipped = (((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) && + (entry2->flags & XFS_ATTR_INCOMPLETE)); + + xfs_trans_brelse(args->trans, bp1); + xfs_trans_brelse(args->trans, bp2); + + return 0; +} diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h index e108b37..12283cf 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.h +++ b/fs/xfs/libxfs/xfs_attr_leaf.h @@ -57,7 +57,9 @@ int xfs_attr3_leaf_to_shortform(struct xfs_buf *bp, struct xfs_da_args *args, int forkoff); int xfs_attr3_leaf_clearflag(struct xfs_da_args *args); int xfs_attr3_leaf_setflag(struct xfs_da_args *args); +int xfs_attr3_leaf_flag_is_set(struct xfs_da_args *args, bool *isset); int xfs_attr3_leaf_flipflags(struct xfs_da_args *args); +int xfs_attr3_leaf_flagsflipped(struct xfs_da_args *args, bool *isflipped); /* * Routines used for growing the Btree.
And new helper functions xfs_attr3_leaf_flag_is_set and xfs_attr3_leaf_flagsflipped. These routines check to see if xfs_attr3_leaf_setflag or xfs_attr3_leaf_flipflags have already been run. We will need this later for delayed attributes since routines may be recalled several times when -EAGAIN is returned. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr_leaf.c | 94 +++++++++++++++++++++++++++++++++++++++++++ fs/xfs/libxfs/xfs_attr_leaf.h | 2 + 2 files changed, 96 insertions(+)