| Message ID | 20190412225036.22939-8-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Delayed Attributes | expand |
On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > and a new state type. We will use these in the next patch when > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > Because the subroutines of this function modify the contents of these > structures, we need to find a place to store them where they remain > instantiated across multiple calls to xfs_set_attr_args. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > fs/xfs/scrub/common.c | 2 ++ > fs/xfs/xfs_acl.c | 2 ++ > fs/xfs/xfs_attr_item.c | 2 +- > fs/xfs/xfs_ioctl.c | 2 ++ > fs/xfs/xfs_ioctl32.c | 2 ++ > fs/xfs/xfs_iops.c | 1 + > fs/xfs/xfs_xattr.c | 1 + > 8 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index 974c963..4ce3b0a 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > char a_name[1]; /* attr name (NULL terminated) */ > } attrlist_ent_t; > > +/* Attr state machine types */ > +enum xfs_attr_state { > + XFS_ATTR_STATE1 = 1, > + XFS_ATTR_STATE2 = 2, > + XFS_ATTR_STATE3 = 3, Um... to what states do these refer? > +}; > + > /* > * List of attrs to commit later. > */ > @@ -88,7 +95,16 @@ struct xfs_attr_item { > void *xattri_name; /* attr name */ > uint32_t xattri_name_len; /* length of name */ > uint32_t xattri_flags; /* attr flags */ > - struct list_head xattri_list; > + > + /* > + * Delayed attr parameters that need to remain instantiated > + * across transaction rolls during the defer finish > + */ > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > + enum xfs_attr_state xattri_state; /* state machine marker */ > + struct xfs_da_args xattri_args; /* args context */ Assuming we're keeping xattri_args.trans up to date here? > + > + struct list_head xattri_list; What's this for? --D > > /* > * A byte array follows the header containing the file name and > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > index 0c54ff5..270c32e 100644 > --- a/fs/xfs/scrub/common.c > +++ b/fs/xfs/scrub/common.c > @@ -30,6 +30,8 @@ > #include "xfs_rmap_btree.h" > #include "xfs_log.h" > #include "xfs_trans_priv.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_reflink.h" > #include "scrub/xfs_scrub.h" > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > index 142de8d..9b1b93e 100644 > --- a/fs/xfs/xfs_acl.c > +++ b/fs/xfs/xfs_acl.c > @@ -10,6 +10,8 @@ > #include "xfs_mount.h" > #include "xfs_inode.h" > #include "xfs_acl.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_trace.h" > #include <linux/slab.h> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > index 0ea19b4..36e6d1e 100644 > --- a/fs/xfs/xfs_attr_item.c > +++ b/fs/xfs/xfs_attr_item.c > @@ -19,10 +19,10 @@ > #include "xfs_rmap.h" > #include "xfs_inode.h" > #include "xfs_icache.h" > -#include "xfs_attr.h" > #include "xfs_shared.h" > #include "xfs_da_format.h" > #include "xfs_da_btree.h" > +#include "xfs_attr.h" > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > { > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index ab341d6..c8728ca 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -16,6 +16,8 @@ > #include "xfs_rtalloc.h" > #include "xfs_itable.h" > #include "xfs_error.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_bmap.h" > #include "xfs_bmap_util.h" > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index 5001dca..23f6990 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -21,6 +21,8 @@ > #include "xfs_fsops.h" > #include "xfs_alloc.h" > #include "xfs_rtalloc.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_ioctl.h" > #include "xfs_ioctl32.h" > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index e73c21a..561c467 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -17,6 +17,7 @@ > #include "xfs_acl.h" > #include "xfs_quota.h" > #include "xfs_error.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_trans.h" > #include "xfs_trace.h" > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > index 3013746..938e81d 100644 > --- a/fs/xfs/xfs_xattr.c > +++ b/fs/xfs/xfs_xattr.c > @@ -11,6 +11,7 @@ > #include "xfs_mount.h" > #include "xfs_da_format.h" > #include "xfs_inode.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_attr_leaf.h" > #include "xfs_acl.h" > -- > 2.7.4 >
On 4/15/19 3:50 PM, wrote: > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: >> This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer >> and a new state type. We will use these in the next patch when >> we modify xfs_set_attr_args to roll transactions by returning EAGAIN. >> Because the subroutines of this function modify the contents of these >> structures, we need to find a place to store them where they remain >> instantiated across multiple calls to xfs_set_attr_args. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- >> fs/xfs/scrub/common.c | 2 ++ >> fs/xfs/xfs_acl.c | 2 ++ >> fs/xfs/xfs_attr_item.c | 2 +- >> fs/xfs/xfs_ioctl.c | 2 ++ >> fs/xfs/xfs_ioctl32.c | 2 ++ >> fs/xfs/xfs_iops.c | 1 + >> fs/xfs/xfs_xattr.c | 1 + >> 8 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >> index 974c963..4ce3b0a 100644 >> --- a/fs/xfs/libxfs/xfs_attr.h >> +++ b/fs/xfs/libxfs/xfs_attr.h >> @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ >> char a_name[1]; /* attr name (NULL terminated) */ >> } attrlist_ent_t; >> >> +/* Attr state machine types */ >> +enum xfs_attr_state { >> + XFS_ATTR_STATE1 = 1, >> + XFS_ATTR_STATE2 = 2, >> + XFS_ATTR_STATE3 = 3, > > Um... to what states do these refer? I actually struggled with what to call these other than state machine types. They are sort of "you were here" bookmark for xfs_attr_set_args. The idea is that when we return EAGAIN, and then get recalled with a new transaction, we jump back to where we were based on this marker. > >> +}; >> + >> /* >> * List of attrs to commit later. >> */ >> @@ -88,7 +95,16 @@ struct xfs_attr_item { >> void *xattri_name; /* attr name */ >> uint32_t xattri_name_len; /* length of name */ >> uint32_t xattri_flags; /* attr flags */ >> - struct list_head xattri_list; >> + >> + /* >> + * Delayed attr parameters that need to remain instantiated >> + * across transaction rolls during the defer finish >> + */ >> + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ >> + enum xfs_attr_state xattri_state; /* state machine marker */ >> + struct xfs_da_args xattri_args; /* args context */ > > Assuming we're keeping xattri_args.trans up to date here? Yes, that happens in xfs_attr_finish_item in the next patch. > >> + >> + struct list_head xattri_list; > > What's this for? xattri_list is introduced in patch 2, which I loosely modeled off other delayed items at the time. It a list of intents that have been logged to this item. Though it could use a comment :-) It doesn't relate directly to the "re-roll with EAGAIN" mechanics being added in this patch if that's what you are asking. It just needs to be the last member of the struct because it's followed by a byte array. I hope that helps to explain some. Let me know if you have any other questions. Thanks! Allison > > --D > >> >> /* >> * A byte array follows the header containing the file name and >> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c >> index 0c54ff5..270c32e 100644 >> --- a/fs/xfs/scrub/common.c >> +++ b/fs/xfs/scrub/common.c >> @@ -30,6 +30,8 @@ >> #include "xfs_rmap_btree.h" >> #include "xfs_log.h" >> #include "xfs_trans_priv.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_reflink.h" >> #include "scrub/xfs_scrub.h" >> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c >> index 142de8d..9b1b93e 100644 >> --- a/fs/xfs/xfs_acl.c >> +++ b/fs/xfs/xfs_acl.c >> @@ -10,6 +10,8 @@ >> #include "xfs_mount.h" >> #include "xfs_inode.h" >> #include "xfs_acl.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_trace.h" >> #include <linux/slab.h> >> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c >> index 0ea19b4..36e6d1e 100644 >> --- a/fs/xfs/xfs_attr_item.c >> +++ b/fs/xfs/xfs_attr_item.c >> @@ -19,10 +19,10 @@ >> #include "xfs_rmap.h" >> #include "xfs_inode.h" >> #include "xfs_icache.h" >> -#include "xfs_attr.h" >> #include "xfs_shared.h" >> #include "xfs_da_format.h" >> #include "xfs_da_btree.h" >> +#include "xfs_attr.h" >> >> static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) >> { >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index ab341d6..c8728ca 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -16,6 +16,8 @@ >> #include "xfs_rtalloc.h" >> #include "xfs_itable.h" >> #include "xfs_error.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_bmap.h" >> #include "xfs_bmap_util.h" >> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c >> index 5001dca..23f6990 100644 >> --- a/fs/xfs/xfs_ioctl32.c >> +++ b/fs/xfs/xfs_ioctl32.c >> @@ -21,6 +21,8 @@ >> #include "xfs_fsops.h" >> #include "xfs_alloc.h" >> #include "xfs_rtalloc.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_ioctl.h" >> #include "xfs_ioctl32.h" >> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c >> index e73c21a..561c467 100644 >> --- a/fs/xfs/xfs_iops.c >> +++ b/fs/xfs/xfs_iops.c >> @@ -17,6 +17,7 @@ >> #include "xfs_acl.h" >> #include "xfs_quota.h" >> #include "xfs_error.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_trans.h" >> #include "xfs_trace.h" >> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c >> index 3013746..938e81d 100644 >> --- a/fs/xfs/xfs_xattr.c >> +++ b/fs/xfs/xfs_xattr.c >> @@ -11,6 +11,7 @@ >> #include "xfs_mount.h" >> #include "xfs_da_format.h" >> #include "xfs_inode.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_attr_leaf.h" >> #include "xfs_acl.h" >> -- >> 2.7.4 >>
On 4/15/19 7:30 PM, Allison Henderson wrote: > On 4/15/19 3:50 PM, wrote: >> On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: >>> This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf >>> pointer >>> and a new state type. We will use these in the next patch when >>> we modify xfs_set_attr_args to roll transactions by returning EAGAIN. >>> Because the subroutines of this function modify the contents of these >>> structures, we need to find a place to store them where they remain >>> instantiated across multiple calls to xfs_set_attr_args. >>> >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>> --- >>> fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- >>> fs/xfs/scrub/common.c | 2 ++ >>> fs/xfs/xfs_acl.c | 2 ++ >>> fs/xfs/xfs_attr_item.c | 2 +- >>> fs/xfs/xfs_ioctl.c | 2 ++ >>> fs/xfs/xfs_ioctl32.c | 2 ++ >>> fs/xfs/xfs_iops.c | 1 + >>> fs/xfs/xfs_xattr.c | 1 + >>> 8 files changed, 28 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >>> index 974c963..4ce3b0a 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.h >>> +++ b/fs/xfs/libxfs/xfs_attr.h >>> @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from >>> attr_list() */ >>> char a_name[1]; /* attr name (NULL terminated) */ >>> } attrlist_ent_t; >>> +/* Attr state machine types */ >>> +enum xfs_attr_state { >>> + XFS_ATTR_STATE1 = 1, >>> + XFS_ATTR_STATE2 = 2, >>> + XFS_ATTR_STATE3 = 3, >> >> Um... to what states do these refer? > > I actually struggled with what to call these other than state machine > types. They are sort of "you were here" bookmark for xfs_attr_set_args. > The idea is that when we return EAGAIN, and then get recalled with a > new transaction, we jump back to where we were based on this marker. > >> >>> +}; >>> + >>> /* >>> * List of attrs to commit later. >>> */ >>> @@ -88,7 +95,16 @@ struct xfs_attr_item { >>> void *xattri_name; /* attr name */ >>> uint32_t xattri_name_len; /* length of name */ >>> uint32_t xattri_flags; /* attr flags */ >>> - struct list_head xattri_list; >>> + >>> + /* >>> + * Delayed attr parameters that need to remain instantiated >>> + * across transaction rolls during the defer finish >>> + */ >>> + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ >>> + enum xfs_attr_state xattri_state; /* state machine >>> marker */ >>> + struct xfs_da_args xattri_args; /* args context */ >> >> Assuming we're keeping xattri_args.trans up to date here? > > Yes, that happens in xfs_attr_finish_item in the next patch. > >> >>> + >>> + struct list_head xattri_list; >> >> What's this for? > > xattri_list is introduced in patch 2, which I loosely modeled off other Sorry, typo: xattri_list is introduced in patch 4, not 2. > delayed items at the time. It a list of intents that have been logged > to this item. Though it could use a comment :-) > > It doesn't relate directly to the "re-roll with EAGAIN" mechanics being > added in this patch if that's what you are asking. It just needs to be > the last member of the struct because it's followed by a byte array. > > I hope that helps to explain some. Let me know if you have any other > questions. Thanks! > > Allison >> >> --D >> >>> /* >>> * A byte array follows the header containing the file name and >>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c >>> index 0c54ff5..270c32e 100644 >>> --- a/fs/xfs/scrub/common.c >>> +++ b/fs/xfs/scrub/common.c >>> @@ -30,6 +30,8 @@ >>> #include "xfs_rmap_btree.h" >>> #include "xfs_log.h" >>> #include "xfs_trans_priv.h" >>> +#include "xfs_da_format.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_reflink.h" >>> #include "scrub/xfs_scrub.h" >>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c >>> index 142de8d..9b1b93e 100644 >>> --- a/fs/xfs/xfs_acl.c >>> +++ b/fs/xfs/xfs_acl.c >>> @@ -10,6 +10,8 @@ >>> #include "xfs_mount.h" >>> #include "xfs_inode.h" >>> #include "xfs_acl.h" >>> +#include "xfs_da_format.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_trace.h" >>> #include <linux/slab.h> >>> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c >>> index 0ea19b4..36e6d1e 100644 >>> --- a/fs/xfs/xfs_attr_item.c >>> +++ b/fs/xfs/xfs_attr_item.c >>> @@ -19,10 +19,10 @@ >>> #include "xfs_rmap.h" >>> #include "xfs_inode.h" >>> #include "xfs_icache.h" >>> -#include "xfs_attr.h" >>> #include "xfs_shared.h" >>> #include "xfs_da_format.h" >>> #include "xfs_da_btree.h" >>> +#include "xfs_attr.h" >>> static inline struct xfs_attri_log_item *ATTRI_ITEM(struct >>> xfs_log_item *lip) >>> { >>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >>> index ab341d6..c8728ca 100644 >>> --- a/fs/xfs/xfs_ioctl.c >>> +++ b/fs/xfs/xfs_ioctl.c >>> @@ -16,6 +16,8 @@ >>> #include "xfs_rtalloc.h" >>> #include "xfs_itable.h" >>> #include "xfs_error.h" >>> +#include "xfs_da_format.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_bmap.h" >>> #include "xfs_bmap_util.h" >>> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c >>> index 5001dca..23f6990 100644 >>> --- a/fs/xfs/xfs_ioctl32.c >>> +++ b/fs/xfs/xfs_ioctl32.c >>> @@ -21,6 +21,8 @@ >>> #include "xfs_fsops.h" >>> #include "xfs_alloc.h" >>> #include "xfs_rtalloc.h" >>> +#include "xfs_da_format.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_ioctl.h" >>> #include "xfs_ioctl32.h" >>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c >>> index e73c21a..561c467 100644 >>> --- a/fs/xfs/xfs_iops.c >>> +++ b/fs/xfs/xfs_iops.c >>> @@ -17,6 +17,7 @@ >>> #include "xfs_acl.h" >>> #include "xfs_quota.h" >>> #include "xfs_error.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_trans.h" >>> #include "xfs_trace.h" >>> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c >>> index 3013746..938e81d 100644 >>> --- a/fs/xfs/xfs_xattr.c >>> +++ b/fs/xfs/xfs_xattr.c >>> @@ -11,6 +11,7 @@ >>> #include "xfs_mount.h" >>> #include "xfs_da_format.h" >>> #include "xfs_inode.h" >>> +#include "xfs_da_btree.h" >>> #include "xfs_attr.h" >>> #include "xfs_attr_leaf.h" >>> #include "xfs_acl.h" >>> -- >>> 2.7.4 >>>
On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > and a new state type. We will use these in the next patch when > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > Because the subroutines of this function modify the contents of these > structures, we need to find a place to store them where they remain > instantiated across multiple calls to xfs_set_attr_args. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- I see Darrick has already commented on the whole state thing. I'll probably have to grok the next patch to comment further, but just a couple initial thoughts: First, I hit a build failure with this patch. It looks like there's a missed include in the scrub code: ... CC [M] fs/xfs/scrub/repair.o In file included from fs/xfs/scrub/repair.c:32: fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type struct xfs_da_args xattri_args; /* args context */ ... Second, the commit log suggests that the states will reflect the current transaction roll points (i.e., establishing re-entry points down in xfs_attr_set_args(). I'm kind of wondering if we should break these xattr set sub-sequences down into smaller helper functions (refactoring the existing code as we go) such that the mechanism could technically be used deferred or not. Re: the previous thought on whether to defer xattr removes or not, there might also be cases where there's not a need to defer xattr sets. E.g., taking a quick peek into the next patch, the state 1 case in xfs_attr_try_sf_addname() is actually a transaction commit, which I think means we're done. We'd have done an attr memory allocation, deferred op and transaction roll where none was necessary so it might not be worth it to defer in that scenario. Hmm, it also looks like we return -EAGAIN in places where we've not actually done any work, like if a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we even attempt the sf add). That kind of looks like a waste of transaction rolls and further suggests it might be cleaner to break this whole path down into helpers and put it back together in a way more conducive to deferred operations. Brian > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > fs/xfs/scrub/common.c | 2 ++ > fs/xfs/xfs_acl.c | 2 ++ > fs/xfs/xfs_attr_item.c | 2 +- > fs/xfs/xfs_ioctl.c | 2 ++ > fs/xfs/xfs_ioctl32.c | 2 ++ > fs/xfs/xfs_iops.c | 1 + > fs/xfs/xfs_xattr.c | 1 + > 8 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index 974c963..4ce3b0a 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > char a_name[1]; /* attr name (NULL terminated) */ > } attrlist_ent_t; > > +/* Attr state machine types */ > +enum xfs_attr_state { > + XFS_ATTR_STATE1 = 1, > + XFS_ATTR_STATE2 = 2, > + XFS_ATTR_STATE3 = 3, > +}; > + > /* > * List of attrs to commit later. > */ > @@ -88,7 +95,16 @@ struct xfs_attr_item { > void *xattri_name; /* attr name */ > uint32_t xattri_name_len; /* length of name */ > uint32_t xattri_flags; /* attr flags */ > - struct list_head xattri_list; > + > + /* > + * Delayed attr parameters that need to remain instantiated > + * across transaction rolls during the defer finish > + */ > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > + enum xfs_attr_state xattri_state; /* state machine marker */ > + struct xfs_da_args xattri_args; /* args context */ > + > + struct list_head xattri_list; > > /* > * A byte array follows the header containing the file name and > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > index 0c54ff5..270c32e 100644 > --- a/fs/xfs/scrub/common.c > +++ b/fs/xfs/scrub/common.c > @@ -30,6 +30,8 @@ > #include "xfs_rmap_btree.h" > #include "xfs_log.h" > #include "xfs_trans_priv.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_reflink.h" > #include "scrub/xfs_scrub.h" > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > index 142de8d..9b1b93e 100644 > --- a/fs/xfs/xfs_acl.c > +++ b/fs/xfs/xfs_acl.c > @@ -10,6 +10,8 @@ > #include "xfs_mount.h" > #include "xfs_inode.h" > #include "xfs_acl.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_trace.h" > #include <linux/slab.h> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > index 0ea19b4..36e6d1e 100644 > --- a/fs/xfs/xfs_attr_item.c > +++ b/fs/xfs/xfs_attr_item.c > @@ -19,10 +19,10 @@ > #include "xfs_rmap.h" > #include "xfs_inode.h" > #include "xfs_icache.h" > -#include "xfs_attr.h" > #include "xfs_shared.h" > #include "xfs_da_format.h" > #include "xfs_da_btree.h" > +#include "xfs_attr.h" > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > { > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index ab341d6..c8728ca 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -16,6 +16,8 @@ > #include "xfs_rtalloc.h" > #include "xfs_itable.h" > #include "xfs_error.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_bmap.h" > #include "xfs_bmap_util.h" > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index 5001dca..23f6990 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -21,6 +21,8 @@ > #include "xfs_fsops.h" > #include "xfs_alloc.h" > #include "xfs_rtalloc.h" > +#include "xfs_da_format.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_ioctl.h" > #include "xfs_ioctl32.h" > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index e73c21a..561c467 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -17,6 +17,7 @@ > #include "xfs_acl.h" > #include "xfs_quota.h" > #include "xfs_error.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_trans.h" > #include "xfs_trace.h" > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > index 3013746..938e81d 100644 > --- a/fs/xfs/xfs_xattr.c > +++ b/fs/xfs/xfs_xattr.c > @@ -11,6 +11,7 @@ > #include "xfs_mount.h" > #include "xfs_da_format.h" > #include "xfs_inode.h" > +#include "xfs_da_btree.h" > #include "xfs_attr.h" > #include "xfs_attr_leaf.h" > #include "xfs_acl.h" > -- > 2.7.4 >
On 4/22/19 6:03 AM, Brian Foster wrote: > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: >> This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer >> and a new state type. We will use these in the next patch when >> we modify xfs_set_attr_args to roll transactions by returning EAGAIN. >> Because the subroutines of this function modify the contents of these >> structures, we need to find a place to store them where they remain >> instantiated across multiple calls to xfs_set_attr_args. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> --- > > I see Darrick has already commented on the whole state thing. I'll > probably have to grok the next patch to comment further, but just a > couple initial thoughts: > > First, I hit a build failure with this patch. It looks like there's a > missed include in the scrub code: > > ... > CC [M] fs/xfs/scrub/repair.o > In file included from fs/xfs/scrub/repair.c:32: > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > struct xfs_da_args xattri_args; /* args context */ Hmm, ok. I'll get that corrected, I probably need to clean out my workspace and build from scratch. > ... > > Second, the commit log suggests that the states will reflect the current > transaction roll points (i.e., establishing re-entry points down in > xfs_attr_set_args(). I'm kind of wondering if we should break these > xattr set sub-sequences down into smaller helper functions (refactoring > the existing code as we go) such that the mechanism could technically be > used deferred or not. Re: the previous thought on whether to defer xattr > removes or not, there might also be cases where there's not a need to > defer xattr sets. > > E.g., taking a quick peek into the next patch, the state 1 case in > xfs_attr_try_sf_addname() is actually a transaction commit, which I > think means we're done. We'd have done an attr memory allocation, > deferred op and transaction roll where none was necessary so it might > not be worth it to defer in that scenario. Hmm, it also looks like we > return -EAGAIN in places where we've not actually done any work, like if > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > even attempt the sf add). That kind of looks like a waste of transaction > rolls and further suggests it might be cleaner to break this whole path > down into helpers and put it back together in a way more conducive to > deferred operations. Yes, this area is a bit of a wart the way it is right now. I think you're right in that ultimately we may end up having to do a lot of refactoring in order to have more efficient "re-entry points". The state machine is hard to get into subroutines, so it's limited in use in the top level function. I was also starting to wonder if maybe I could do some refactoring in xfs_defer_finish_noroll to capture the common code associated with the -EAGAIN handling. Then maybe we could make a function pointer that we can pass through the finish_item interface. The idea being that subroutines could use the function pointer to cycle out the transaction when needed instead of having to record states and back out like this. It'd be a new parameter to pipe around, but it'd be more efficient than the state machine, and less surgery in the refactor. And maybe a blessing to any other operations that might need to go through this transition in the future. Thoughts? Thanks again for the reviews! Allison > > Brian > > >> fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- >> fs/xfs/scrub/common.c | 2 ++ >> fs/xfs/xfs_acl.c | 2 ++ >> fs/xfs/xfs_attr_item.c | 2 +- >> fs/xfs/xfs_ioctl.c | 2 ++ >> fs/xfs/xfs_ioctl32.c | 2 ++ >> fs/xfs/xfs_iops.c | 1 + >> fs/xfs/xfs_xattr.c | 1 + >> 8 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >> index 974c963..4ce3b0a 100644 >> --- a/fs/xfs/libxfs/xfs_attr.h >> +++ b/fs/xfs/libxfs/xfs_attr.h >> @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ >> char a_name[1]; /* attr name (NULL terminated) */ >> } attrlist_ent_t; >> >> +/* Attr state machine types */ >> +enum xfs_attr_state { >> + XFS_ATTR_STATE1 = 1, >> + XFS_ATTR_STATE2 = 2, >> + XFS_ATTR_STATE3 = 3, >> +}; >> + >> /* >> * List of attrs to commit later. >> */ >> @@ -88,7 +95,16 @@ struct xfs_attr_item { >> void *xattri_name; /* attr name */ >> uint32_t xattri_name_len; /* length of name */ >> uint32_t xattri_flags; /* attr flags */ >> - struct list_head xattri_list; >> + >> + /* >> + * Delayed attr parameters that need to remain instantiated >> + * across transaction rolls during the defer finish >> + */ >> + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ >> + enum xfs_attr_state xattri_state; /* state machine marker */ >> + struct xfs_da_args xattri_args; /* args context */ >> + >> + struct list_head xattri_list; >> >> /* >> * A byte array follows the header containing the file name and >> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c >> index 0c54ff5..270c32e 100644 >> --- a/fs/xfs/scrub/common.c >> +++ b/fs/xfs/scrub/common.c >> @@ -30,6 +30,8 @@ >> #include "xfs_rmap_btree.h" >> #include "xfs_log.h" >> #include "xfs_trans_priv.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_reflink.h" >> #include "scrub/xfs_scrub.h" >> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c >> index 142de8d..9b1b93e 100644 >> --- a/fs/xfs/xfs_acl.c >> +++ b/fs/xfs/xfs_acl.c >> @@ -10,6 +10,8 @@ >> #include "xfs_mount.h" >> #include "xfs_inode.h" >> #include "xfs_acl.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_trace.h" >> #include <linux/slab.h> >> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c >> index 0ea19b4..36e6d1e 100644 >> --- a/fs/xfs/xfs_attr_item.c >> +++ b/fs/xfs/xfs_attr_item.c >> @@ -19,10 +19,10 @@ >> #include "xfs_rmap.h" >> #include "xfs_inode.h" >> #include "xfs_icache.h" >> -#include "xfs_attr.h" >> #include "xfs_shared.h" >> #include "xfs_da_format.h" >> #include "xfs_da_btree.h" >> +#include "xfs_attr.h" >> >> static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) >> { >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index ab341d6..c8728ca 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -16,6 +16,8 @@ >> #include "xfs_rtalloc.h" >> #include "xfs_itable.h" >> #include "xfs_error.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_bmap.h" >> #include "xfs_bmap_util.h" >> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c >> index 5001dca..23f6990 100644 >> --- a/fs/xfs/xfs_ioctl32.c >> +++ b/fs/xfs/xfs_ioctl32.c >> @@ -21,6 +21,8 @@ >> #include "xfs_fsops.h" >> #include "xfs_alloc.h" >> #include "xfs_rtalloc.h" >> +#include "xfs_da_format.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_ioctl.h" >> #include "xfs_ioctl32.h" >> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c >> index e73c21a..561c467 100644 >> --- a/fs/xfs/xfs_iops.c >> +++ b/fs/xfs/xfs_iops.c >> @@ -17,6 +17,7 @@ >> #include "xfs_acl.h" >> #include "xfs_quota.h" >> #include "xfs_error.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_trans.h" >> #include "xfs_trace.h" >> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c >> index 3013746..938e81d 100644 >> --- a/fs/xfs/xfs_xattr.c >> +++ b/fs/xfs/xfs_xattr.c >> @@ -11,6 +11,7 @@ >> #include "xfs_mount.h" >> #include "xfs_da_format.h" >> #include "xfs_inode.h" >> +#include "xfs_da_btree.h" >> #include "xfs_attr.h" >> #include "xfs_attr_leaf.h" >> #include "xfs_acl.h" >> -- >> 2.7.4 >>
On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: > > > On 4/22/19 6:03 AM, Brian Foster wrote: > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > > > and a new state type. We will use these in the next patch when > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > > > Because the subroutines of this function modify the contents of these > > > structures, we need to find a place to store them where they remain > > > instantiated across multiple calls to xfs_set_attr_args. > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > --- > > > > I see Darrick has already commented on the whole state thing. I'll > > probably have to grok the next patch to comment further, but just a > > couple initial thoughts: > > > > First, I hit a build failure with this patch. It looks like there's a > > missed include in the scrub code: > > > > ... > > CC [M] fs/xfs/scrub/repair.o > > In file included from fs/xfs/scrub/repair.c:32: > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > > struct xfs_da_args xattri_args; /* args context */ > Hmm, ok. I'll get that corrected, I probably need to clean out my workspace > and build from scratch. > > > ... > > > > Second, the commit log suggests that the states will reflect the current > > transaction roll points (i.e., establishing re-entry points down in > > xfs_attr_set_args(). I'm kind of wondering if we should break these > > xattr set sub-sequences down into smaller helper functions (refactoring > > the existing code as we go) such that the mechanism could technically be > > used deferred or not. Re: the previous thought on whether to defer xattr > > removes or not, there might also be cases where there's not a need to > > defer xattr sets. > > > > E.g., taking a quick peek into the next patch, the state 1 case in > > xfs_attr_try_sf_addname() is actually a transaction commit, which I > > think means we're done. We'd have done an attr memory allocation, > > deferred op and transaction roll where none was necessary so it might > > not be worth it to defer in that scenario. Hmm, it also looks like we > > return -EAGAIN in places where we've not actually done any work, like if > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > > even attempt the sf add). That kind of looks like a waste of transaction > > rolls and further suggests it might be cleaner to break this whole path > > down into helpers and put it back together in a way more conducive to > > deferred operations. > > Yes, this area is a bit of a wart the way it is right now. I think you're > right in that ultimately we may end up having to do a lot of refactoring in > order to have more efficient "re-entry points". The state machine is hard > to get into subroutines, so it's limited in use in the top level function. > > I was also starting to wonder if maybe I could do some refactoring in > xfs_defer_finish_noroll to capture the common code associated with the > -EAGAIN handling. Then maybe we could make a function pointer that we can > pass through the finish_item interface. The idea being that subroutines > could use the function pointer to cycle out the transaction when needed > instead of having to record states and back out like this. It'd be a new > parameter to pipe around, but it'd be more efficient than the state machine, > and less surgery in the refactor. And maybe a blessing to any other > operations that might need to go through this transition in the future. > Thoughts? > That's an interesting idea. It still strikes me as a bit of a fallback/hack as opposed to organizing the code to properly fit into the dfops infrastructure, but it could be useful as a transient solution. From a high level, it looks like we'd have to create a new intent, relog this item and all remaining items associated with the dfp to it, roll the tx, and finally create a done item associated with the intent in the new tx. You'd need access to the dfp for some of that, so it's not immediately clear to me that this ends up much easier than fixing up the xattr code. BTW, if we did end up with something like that I'd probably prefer to see it as an exported dfops helper function as opposed to a function pointer being passed around, if possible. Brian > Thanks again for the reviews! > > Allison > > > > > Brian > > > > > > > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > > > fs/xfs/scrub/common.c | 2 ++ > > > fs/xfs/xfs_acl.c | 2 ++ > > > fs/xfs/xfs_attr_item.c | 2 +- > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > fs/xfs/xfs_ioctl32.c | 2 ++ > > > fs/xfs/xfs_iops.c | 1 + > > > fs/xfs/xfs_xattr.c | 1 + > > > 8 files changed, 28 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > index 974c963..4ce3b0a 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > > > char a_name[1]; /* attr name (NULL terminated) */ > > > } attrlist_ent_t; > > > +/* Attr state machine types */ > > > +enum xfs_attr_state { > > > + XFS_ATTR_STATE1 = 1, > > > + XFS_ATTR_STATE2 = 2, > > > + XFS_ATTR_STATE3 = 3, > > > +}; > > > + > > > /* > > > * List of attrs to commit later. > > > */ > > > @@ -88,7 +95,16 @@ struct xfs_attr_item { > > > void *xattri_name; /* attr name */ > > > uint32_t xattri_name_len; /* length of name */ > > > uint32_t xattri_flags; /* attr flags */ > > > - struct list_head xattri_list; > > > + > > > + /* > > > + * Delayed attr parameters that need to remain instantiated > > > + * across transaction rolls during the defer finish > > > + */ > > > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > > > + enum xfs_attr_state xattri_state; /* state machine marker */ > > > + struct xfs_da_args xattri_args; /* args context */ > > > + > > > + struct list_head xattri_list; > > > /* > > > * A byte array follows the header containing the file name and > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > > > index 0c54ff5..270c32e 100644 > > > --- a/fs/xfs/scrub/common.c > > > +++ b/fs/xfs/scrub/common.c > > > @@ -30,6 +30,8 @@ > > > #include "xfs_rmap_btree.h" > > > #include "xfs_log.h" > > > #include "xfs_trans_priv.h" > > > +#include "xfs_da_format.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_reflink.h" > > > #include "scrub/xfs_scrub.h" > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > > index 142de8d..9b1b93e 100644 > > > --- a/fs/xfs/xfs_acl.c > > > +++ b/fs/xfs/xfs_acl.c > > > @@ -10,6 +10,8 @@ > > > #include "xfs_mount.h" > > > #include "xfs_inode.h" > > > #include "xfs_acl.h" > > > +#include "xfs_da_format.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_trace.h" > > > #include <linux/slab.h> > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > > index 0ea19b4..36e6d1e 100644 > > > --- a/fs/xfs/xfs_attr_item.c > > > +++ b/fs/xfs/xfs_attr_item.c > > > @@ -19,10 +19,10 @@ > > > #include "xfs_rmap.h" > > > #include "xfs_inode.h" > > > #include "xfs_icache.h" > > > -#include "xfs_attr.h" > > > #include "xfs_shared.h" > > > #include "xfs_da_format.h" > > > #include "xfs_da_btree.h" > > > +#include "xfs_attr.h" > > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > > > { > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > index ab341d6..c8728ca 100644 > > > --- a/fs/xfs/xfs_ioctl.c > > > +++ b/fs/xfs/xfs_ioctl.c > > > @@ -16,6 +16,8 @@ > > > #include "xfs_rtalloc.h" > > > #include "xfs_itable.h" > > > #include "xfs_error.h" > > > +#include "xfs_da_format.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_bmap.h" > > > #include "xfs_bmap_util.h" > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > > > index 5001dca..23f6990 100644 > > > --- a/fs/xfs/xfs_ioctl32.c > > > +++ b/fs/xfs/xfs_ioctl32.c > > > @@ -21,6 +21,8 @@ > > > #include "xfs_fsops.h" > > > #include "xfs_alloc.h" > > > #include "xfs_rtalloc.h" > > > +#include "xfs_da_format.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_ioctl.h" > > > #include "xfs_ioctl32.h" > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > index e73c21a..561c467 100644 > > > --- a/fs/xfs/xfs_iops.c > > > +++ b/fs/xfs/xfs_iops.c > > > @@ -17,6 +17,7 @@ > > > #include "xfs_acl.h" > > > #include "xfs_quota.h" > > > #include "xfs_error.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_trans.h" > > > #include "xfs_trace.h" > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > > index 3013746..938e81d 100644 > > > --- a/fs/xfs/xfs_xattr.c > > > +++ b/fs/xfs/xfs_xattr.c > > > @@ -11,6 +11,7 @@ > > > #include "xfs_mount.h" > > > #include "xfs_da_format.h" > > > #include "xfs_inode.h" > > > +#include "xfs_da_btree.h" > > > #include "xfs_attr.h" > > > #include "xfs_attr_leaf.h" > > > #include "xfs_acl.h" > > > -- > > > 2.7.4 > > >
On 4/23/19 6:20 AM, Brian Foster wrote: > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: >> >> >> On 4/22/19 6:03 AM, Brian Foster wrote: >>> On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: >>>> This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer >>>> and a new state type. We will use these in the next patch when >>>> we modify xfs_set_attr_args to roll transactions by returning EAGAIN. >>>> Because the subroutines of this function modify the contents of these >>>> structures, we need to find a place to store them where they remain >>>> instantiated across multiple calls to xfs_set_attr_args. >>>> >>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>>> --- >>> >>> I see Darrick has already commented on the whole state thing. I'll >>> probably have to grok the next patch to comment further, but just a >>> couple initial thoughts: >>> >>> First, I hit a build failure with this patch. It looks like there's a >>> missed include in the scrub code: >>> >>> ... >>> CC [M] fs/xfs/scrub/repair.o >>> In file included from fs/xfs/scrub/repair.c:32: >>> fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type >>> struct xfs_da_args xattri_args; /* args context */ >> Hmm, ok. I'll get that corrected, I probably need to clean out my workspace >> and build from scratch. >> >>> ... >>> >>> Second, the commit log suggests that the states will reflect the current >>> transaction roll points (i.e., establishing re-entry points down in >>> xfs_attr_set_args(). I'm kind of wondering if we should break these >>> xattr set sub-sequences down into smaller helper functions (refactoring >>> the existing code as we go) such that the mechanism could technically be >>> used deferred or not. Re: the previous thought on whether to defer xattr >>> removes or not, there might also be cases where there's not a need to >>> defer xattr sets. >>> >>> E.g., taking a quick peek into the next patch, the state 1 case in >>> xfs_attr_try_sf_addname() is actually a transaction commit, which I >>> think means we're done. We'd have done an attr memory allocation, >>> deferred op and transaction roll where none was necessary so it might >>> not be worth it to defer in that scenario. Hmm, it also looks like we >>> return -EAGAIN in places where we've not actually done any work, like if >>> a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we >>> even attempt the sf add). That kind of looks like a waste of transaction >>> rolls and further suggests it might be cleaner to break this whole path >>> down into helpers and put it back together in a way more conducive to >>> deferred operations. >> >> Yes, this area is a bit of a wart the way it is right now. I think you're >> right in that ultimately we may end up having to do a lot of refactoring in >> order to have more efficient "re-entry points". The state machine is hard >> to get into subroutines, so it's limited in use in the top level function. >> >> I was also starting to wonder if maybe I could do some refactoring in >> xfs_defer_finish_noroll to capture the common code associated with the >> -EAGAIN handling. Then maybe we could make a function pointer that we can >> pass through the finish_item interface. The idea being that subroutines >> could use the function pointer to cycle out the transaction when needed >> instead of having to record states and back out like this. It'd be a new >> parameter to pipe around, but it'd be more efficient than the state machine, >> and less surgery in the refactor. And maybe a blessing to any other >> operations that might need to go through this transition in the future. >> Thoughts? >> > > That's an interesting idea. It still strikes me as a bit of a > fallback/hack as opposed to organizing the code to properly fit into the > dfops infrastructure, but it could be useful as a transient solution. > From a high level, it looks like we'd have to create a new intent, relog > this item and all remaining items associated with the dfp to it, roll > the tx, and finally create a done item associated with the intent in the > new tx. You'd need access to the dfp for some of that, so it's not > immediately clear to me that this ends up much easier than fixing up > the xattr code. > > BTW, if we did end up with something like that I'd probably prefer to > see it as an exported dfops helper function as opposed to a function > pointer being passed around, if possible. > Alrighty, I think for now I may try to pursue something more like what you proposed in the next patch and see where I get first. Maybe I'll come back to this later if for some reason it doesn't work out, but I think what you have there is reasonable. Thanks again for the reviews! Allison > Brian > >> Thanks again for the reviews! >> >> Allison >> >>> >>> Brian >>> >>> >>>> fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- >>>> fs/xfs/scrub/common.c | 2 ++ >>>> fs/xfs/xfs_acl.c | 2 ++ >>>> fs/xfs/xfs_attr_item.c | 2 +- >>>> fs/xfs/xfs_ioctl.c | 2 ++ >>>> fs/xfs/xfs_ioctl32.c | 2 ++ >>>> fs/xfs/xfs_iops.c | 1 + >>>> fs/xfs/xfs_xattr.c | 1 + >>>> 8 files changed, 28 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >>>> index 974c963..4ce3b0a 100644 >>>> --- a/fs/xfs/libxfs/xfs_attr.h >>>> +++ b/fs/xfs/libxfs/xfs_attr.h >>>> @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ >>>> char a_name[1]; /* attr name (NULL terminated) */ >>>> } attrlist_ent_t; >>>> +/* Attr state machine types */ >>>> +enum xfs_attr_state { >>>> + XFS_ATTR_STATE1 = 1, >>>> + XFS_ATTR_STATE2 = 2, >>>> + XFS_ATTR_STATE3 = 3, >>>> +}; >>>> + >>>> /* >>>> * List of attrs to commit later. >>>> */ >>>> @@ -88,7 +95,16 @@ struct xfs_attr_item { >>>> void *xattri_name; /* attr name */ >>>> uint32_t xattri_name_len; /* length of name */ >>>> uint32_t xattri_flags; /* attr flags */ >>>> - struct list_head xattri_list; >>>> + >>>> + /* >>>> + * Delayed attr parameters that need to remain instantiated >>>> + * across transaction rolls during the defer finish >>>> + */ >>>> + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ >>>> + enum xfs_attr_state xattri_state; /* state machine marker */ >>>> + struct xfs_da_args xattri_args; /* args context */ >>>> + >>>> + struct list_head xattri_list; >>>> /* >>>> * A byte array follows the header containing the file name and >>>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c >>>> index 0c54ff5..270c32e 100644 >>>> --- a/fs/xfs/scrub/common.c >>>> +++ b/fs/xfs/scrub/common.c >>>> @@ -30,6 +30,8 @@ >>>> #include "xfs_rmap_btree.h" >>>> #include "xfs_log.h" >>>> #include "xfs_trans_priv.h" >>>> +#include "xfs_da_format.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_reflink.h" >>>> #include "scrub/xfs_scrub.h" >>>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c >>>> index 142de8d..9b1b93e 100644 >>>> --- a/fs/xfs/xfs_acl.c >>>> +++ b/fs/xfs/xfs_acl.c >>>> @@ -10,6 +10,8 @@ >>>> #include "xfs_mount.h" >>>> #include "xfs_inode.h" >>>> #include "xfs_acl.h" >>>> +#include "xfs_da_format.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_trace.h" >>>> #include <linux/slab.h> >>>> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c >>>> index 0ea19b4..36e6d1e 100644 >>>> --- a/fs/xfs/xfs_attr_item.c >>>> +++ b/fs/xfs/xfs_attr_item.c >>>> @@ -19,10 +19,10 @@ >>>> #include "xfs_rmap.h" >>>> #include "xfs_inode.h" >>>> #include "xfs_icache.h" >>>> -#include "xfs_attr.h" >>>> #include "xfs_shared.h" >>>> #include "xfs_da_format.h" >>>> #include "xfs_da_btree.h" >>>> +#include "xfs_attr.h" >>>> static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) >>>> { >>>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >>>> index ab341d6..c8728ca 100644 >>>> --- a/fs/xfs/xfs_ioctl.c >>>> +++ b/fs/xfs/xfs_ioctl.c >>>> @@ -16,6 +16,8 @@ >>>> #include "xfs_rtalloc.h" >>>> #include "xfs_itable.h" >>>> #include "xfs_error.h" >>>> +#include "xfs_da_format.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_bmap.h" >>>> #include "xfs_bmap_util.h" >>>> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c >>>> index 5001dca..23f6990 100644 >>>> --- a/fs/xfs/xfs_ioctl32.c >>>> +++ b/fs/xfs/xfs_ioctl32.c >>>> @@ -21,6 +21,8 @@ >>>> #include "xfs_fsops.h" >>>> #include "xfs_alloc.h" >>>> #include "xfs_rtalloc.h" >>>> +#include "xfs_da_format.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_ioctl.h" >>>> #include "xfs_ioctl32.h" >>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c >>>> index e73c21a..561c467 100644 >>>> --- a/fs/xfs/xfs_iops.c >>>> +++ b/fs/xfs/xfs_iops.c >>>> @@ -17,6 +17,7 @@ >>>> #include "xfs_acl.h" >>>> #include "xfs_quota.h" >>>> #include "xfs_error.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_trans.h" >>>> #include "xfs_trace.h" >>>> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c >>>> index 3013746..938e81d 100644 >>>> --- a/fs/xfs/xfs_xattr.c >>>> +++ b/fs/xfs/xfs_xattr.c >>>> @@ -11,6 +11,7 @@ >>>> #include "xfs_mount.h" >>>> #include "xfs_da_format.h" >>>> #include "xfs_inode.h" >>>> +#include "xfs_da_btree.h" >>>> #include "xfs_attr.h" >>>> #include "xfs_attr_leaf.h" >>>> #include "xfs_acl.h" >>>> -- >>>> 2.7.4 >>>>
Sorry I'm late back to the party... On Tue, Apr 23, 2019 at 07:24:40PM -0700, Allison Henderson wrote: > > On 4/23/19 6:20 AM, Brian Foster wrote: > > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: > > > > > > > > > On 4/22/19 6:03 AM, Brian Foster wrote: > > > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > > > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > > > > > and a new state type. We will use these in the next patch when > > > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > > > > > Because the subroutines of this function modify the contents of these > > > > > structures, we need to find a place to store them where they remain > > > > > instantiated across multiple calls to xfs_set_attr_args. > > > > > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > > --- > > > > > > > > I see Darrick has already commented on the whole state thing. I'll > > > > probably have to grok the next patch to comment further, but just a > > > > couple initial thoughts: > > > > > > > > First, I hit a build failure with this patch. It looks like there's a > > > > missed include in the scrub code: > > > > > > > > ... > > > > CC [M] fs/xfs/scrub/repair.o > > > > In file included from fs/xfs/scrub/repair.c:32: > > > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > > > > struct xfs_da_args xattri_args; /* args context */ > > > Hmm, ok. I'll get that corrected, I probably need to clean out my workspace > > > and build from scratch. > > > > > > > ... > > > > > > > > Second, the commit log suggests that the states will reflect the current > > > > transaction roll points (i.e., establishing re-entry points down in > > > > xfs_attr_set_args(). I'm kind of wondering if we should break these > > > > xattr set sub-sequences down into smaller helper functions (refactoring > > > > the existing code as we go) such that the mechanism could technically be I had had the thought of "why not just give each step of setting an attribute its own log item, so we don't have to have this STATE_NNN business?" but then realized that will generate an insane amount of boilerplate, and you're already close to a better solution, so I shut up to think harder. :) > > > > used deferred or not. Re: the previous thought on whether to defer xattr > > > > removes or not, there might also be cases where there's not a need to > > > > defer xattr sets. > > > > > > > > E.g., taking a quick peek into the next patch, the state 1 case in > > > > xfs_attr_try_sf_addname() is actually a transaction commit, which I > > > > think means we're done. We'd have done an attr memory allocation, > > > > deferred op and transaction roll where none was necessary so it might > > > > not be worth it to defer in that scenario. Hmm, it also looks like we > > > > return -EAGAIN in places where we've not actually done any work, like if > > > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > > > > even attempt the sf add). That kind of looks like a waste of transaction > > > > rolls and further suggests it might be cleaner to break this whole path > > > > down into helpers and put it back together in a way more conducive to > > > > deferred operations. Er, agreed: > > > Yes, this area is a bit of a wart the way it is right now. I think you're > > > right in that ultimately we may end up having to do a lot of refactoring in > > > order to have more efficient "re-entry points". The state machine is hard > > > to get into subroutines, so it's limited in use in the top level function. So my current understanding of the problem is that we have this big old xfs_attr_set_args function that does multiple responsibilities requiring transaction rolls, which we can't do directly inside a ->finish_item handler: 1. If no attr fork, add one. 2. If shortform attr fork, try to put it in the sf area. 3. If shortform attr fork and out of space, convert to leaf format. 4. Add attr to leaf/node attr tree. So how about this: refactor each of these pieces into a separate function, then add a separate XFS_ATTR_OP_FLAGS_* value for each of these little pieces. xfs_trans_attr() can call the appropriate little function for the OP_FLAG and xfs_attr_finish_item can figure out which state comes next based on the return value. By directly mapping distinct OP_FLAGS to each piece of the attr setting puzzle, you can use the existing "roll and come back" part of the defer ops machinery. If _finish_item thinks we're done then we just exit. Otherwise, store the new state in the (struct xfs_attr_item *) parameter passed into _finish_item and return -EAGAIN, which puts the defer item back on the defer op list, logs a new xattr intent with the new state, rolls the transaction, and tries to finish the attr again. I think you've already done this last part. xfs_attri_recover then becomes much simpler -- we're passed in the reconstructed log item from which we figure out which step we need to do. We call xfs_trans_attr() to do that one step, but unlike _finish_item, we use the new state to construct a *new* attr intent and attach it to the transaction, then call xfs_defer_move at the end to move all the queued defer_ops to the parent_tp because log recovery requires us to recover all the incomplete log intent items before finishing any new ones that were created as part of recovery. This does mean that we end up with dramatically separate code paths for defer ops attr setting vs. regular attr setting, but as you point out the parent pointer feature will give the new code paths plenty of exercise. Tying the new log intent items to a new feature bit is key to preventing old kernels from stumbling across our new intent items, so we needed to preserve the old attr set paths anyway. Anyway, if this all seems confusing, you can track me down, because I wrote most of this system and therefore have forgotten all of it^W^W^W^W^Wam available to help. :) > > > > > > I was also starting to wonder if maybe I could do some refactoring in > > > xfs_defer_finish_noroll to capture the common code associated with the > > > -EAGAIN handling. Then maybe we could make a function pointer that we can > > > pass through the finish_item interface. The idea being that subroutines > > > could use the function pointer to cycle out the transaction when needed > > > instead of having to record states and back out like this. It'd be a new The state tracking and rolling is already built into xfs_defer.c. :) > > > parameter to pipe around, but it'd be more efficient than the state machine, > > > and less surgery in the refactor. And maybe a blessing to any other > > > operations that might need to go through this transition in the future. > > > Thoughts? > > > > > > > That's an interesting idea. It still strikes me as a bit of a > > fallback/hack as opposed to organizing the code to properly fit into the > > dfops infrastructure, but it could be useful as a transient solution. > > From a high level, it looks like we'd have to create a new intent, relog > > this item and all remaining items associated with the dfp to it, roll > > the tx, and finally create a done item associated with the intent in the > > new tx. You'd need access to the dfp for some of that, so it's not > > immediately clear to me that this ends up much easier than fixing up > > the xattr code. (I think the code that handles EAGAIN being returned from finish_item does this for you....) > > > > BTW, if we did end up with something like that I'd probably prefer to > > see it as an exported dfops helper function as opposed to a function > > pointer being passed around, if possible. > > > > Alrighty, I think for now I may try to pursue something more like what you > proposed in the next patch and see where I get first. Maybe I'll come back > to this later if for some reason it doesn't work out, but I think what you > have there is reasonable. <nod> --D > > Thanks again for the reviews! > Allison > > > Brian > > > > > Thanks again for the reviews! > > > > > > Allison > > > > > > > > > > > Brian > > > > > > > > > > > > > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > > > > > fs/xfs/scrub/common.c | 2 ++ > > > > > fs/xfs/xfs_acl.c | 2 ++ > > > > > fs/xfs/xfs_attr_item.c | 2 +- > > > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > > > fs/xfs/xfs_ioctl32.c | 2 ++ > > > > > fs/xfs/xfs_iops.c | 1 + > > > > > fs/xfs/xfs_xattr.c | 1 + > > > > > 8 files changed, 28 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > index 974c963..4ce3b0a 100644 > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > > > > > char a_name[1]; /* attr name (NULL terminated) */ > > > > > } attrlist_ent_t; > > > > > +/* Attr state machine types */ > > > > > +enum xfs_attr_state { > > > > > + XFS_ATTR_STATE1 = 1, > > > > > + XFS_ATTR_STATE2 = 2, > > > > > + XFS_ATTR_STATE3 = 3, > > > > > +}; > > > > > + > > > > > /* > > > > > * List of attrs to commit later. > > > > > */ > > > > > @@ -88,7 +95,16 @@ struct xfs_attr_item { > > > > > void *xattri_name; /* attr name */ > > > > > uint32_t xattri_name_len; /* length of name */ > > > > > uint32_t xattri_flags; /* attr flags */ > > > > > - struct list_head xattri_list; > > > > > + > > > > > + /* > > > > > + * Delayed attr parameters that need to remain instantiated > > > > > + * across transaction rolls during the defer finish > > > > > + */ > > > > > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > > > > > + enum xfs_attr_state xattri_state; /* state machine marker */ > > > > > + struct xfs_da_args xattri_args; /* args context */ > > > > > + > > > > > + struct list_head xattri_list; > > > > > /* > > > > > * A byte array follows the header containing the file name and > > > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > > > > > index 0c54ff5..270c32e 100644 > > > > > --- a/fs/xfs/scrub/common.c > > > > > +++ b/fs/xfs/scrub/common.c > > > > > @@ -30,6 +30,8 @@ > > > > > #include "xfs_rmap_btree.h" > > > > > #include "xfs_log.h" > > > > > #include "xfs_trans_priv.h" > > > > > +#include "xfs_da_format.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_reflink.h" > > > > > #include "scrub/xfs_scrub.h" > > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > > > > index 142de8d..9b1b93e 100644 > > > > > --- a/fs/xfs/xfs_acl.c > > > > > +++ b/fs/xfs/xfs_acl.c > > > > > @@ -10,6 +10,8 @@ > > > > > #include "xfs_mount.h" > > > > > #include "xfs_inode.h" > > > > > #include "xfs_acl.h" > > > > > +#include "xfs_da_format.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_trace.h" > > > > > #include <linux/slab.h> > > > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > > > > index 0ea19b4..36e6d1e 100644 > > > > > --- a/fs/xfs/xfs_attr_item.c > > > > > +++ b/fs/xfs/xfs_attr_item.c > > > > > @@ -19,10 +19,10 @@ > > > > > #include "xfs_rmap.h" > > > > > #include "xfs_inode.h" > > > > > #include "xfs_icache.h" > > > > > -#include "xfs_attr.h" > > > > > #include "xfs_shared.h" > > > > > #include "xfs_da_format.h" > > > > > #include "xfs_da_btree.h" > > > > > +#include "xfs_attr.h" > > > > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > > > > > { > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > > > index ab341d6..c8728ca 100644 > > > > > --- a/fs/xfs/xfs_ioctl.c > > > > > +++ b/fs/xfs/xfs_ioctl.c > > > > > @@ -16,6 +16,8 @@ > > > > > #include "xfs_rtalloc.h" > > > > > #include "xfs_itable.h" > > > > > #include "xfs_error.h" > > > > > +#include "xfs_da_format.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_bmap.h" > > > > > #include "xfs_bmap_util.h" > > > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > > > > > index 5001dca..23f6990 100644 > > > > > --- a/fs/xfs/xfs_ioctl32.c > > > > > +++ b/fs/xfs/xfs_ioctl32.c > > > > > @@ -21,6 +21,8 @@ > > > > > #include "xfs_fsops.h" > > > > > #include "xfs_alloc.h" > > > > > #include "xfs_rtalloc.h" > > > > > +#include "xfs_da_format.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_ioctl.h" > > > > > #include "xfs_ioctl32.h" > > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > > > index e73c21a..561c467 100644 > > > > > --- a/fs/xfs/xfs_iops.c > > > > > +++ b/fs/xfs/xfs_iops.c > > > > > @@ -17,6 +17,7 @@ > > > > > #include "xfs_acl.h" > > > > > #include "xfs_quota.h" > > > > > #include "xfs_error.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_trans.h" > > > > > #include "xfs_trace.h" > > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > > > > index 3013746..938e81d 100644 > > > > > --- a/fs/xfs/xfs_xattr.c > > > > > +++ b/fs/xfs/xfs_xattr.c > > > > > @@ -11,6 +11,7 @@ > > > > > #include "xfs_mount.h" > > > > > #include "xfs_da_format.h" > > > > > #include "xfs_inode.h" > > > > > +#include "xfs_da_btree.h" > > > > > #include "xfs_attr.h" > > > > > #include "xfs_attr_leaf.h" > > > > > #include "xfs_acl.h" > > > > > -- > > > > > 2.7.4 > > > > >
On Tue, Apr 23, 2019 at 09:10:16PM -0700, Darrick J. Wong wrote: > Sorry I'm late back to the party... > > On Tue, Apr 23, 2019 at 07:24:40PM -0700, Allison Henderson wrote: > > > > On 4/23/19 6:20 AM, Brian Foster wrote: > > > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: > > > > > > > > > > > > On 4/22/19 6:03 AM, Brian Foster wrote: > > > > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > > > > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > > > > > > and a new state type. We will use these in the next patch when > > > > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > > > > > > Because the subroutines of this function modify the contents of these > > > > > > structures, we need to find a place to store them where they remain > > > > > > instantiated across multiple calls to xfs_set_attr_args. > > > > > > > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > > > --- > > > > > > > > > > I see Darrick has already commented on the whole state thing. I'll > > > > > probably have to grok the next patch to comment further, but just a > > > > > couple initial thoughts: > > > > > > > > > > First, I hit a build failure with this patch. It looks like there's a > > > > > missed include in the scrub code: > > > > > > > > > > ... > > > > > CC [M] fs/xfs/scrub/repair.o > > > > > In file included from fs/xfs/scrub/repair.c:32: > > > > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > > > > > struct xfs_da_args xattri_args; /* args context */ > > > > Hmm, ok. I'll get that corrected, I probably need to clean out my workspace > > > > and build from scratch. > > > > > > > > > ... > > > > > > > > > > Second, the commit log suggests that the states will reflect the current > > > > > transaction roll points (i.e., establishing re-entry points down in > > > > > xfs_attr_set_args(). I'm kind of wondering if we should break these > > > > > xattr set sub-sequences down into smaller helper functions (refactoring > > > > > the existing code as we go) such that the mechanism could technically be > > I had had the thought of "why not just give each step of setting an > attribute its own log item, so we don't have to have this STATE_NNN > business?" but then realized that will generate an insane amount of > boilerplate, and you're already close to a better solution, so I shut up > to think harder. :) > The thought of separating things down into smaller "ops" popped into my head (not necessarily separate/smaller log items), but I hadn't really thought it through to this point... > > > > > used deferred or not. Re: the previous thought on whether to defer xattr > > > > > removes or not, there might also be cases where there's not a need to > > > > > defer xattr sets. > > > > > > > > > > E.g., taking a quick peek into the next patch, the state 1 case in > > > > > xfs_attr_try_sf_addname() is actually a transaction commit, which I > > > > > think means we're done. We'd have done an attr memory allocation, > > > > > deferred op and transaction roll where none was necessary so it might > > > > > not be worth it to defer in that scenario. Hmm, it also looks like we > > > > > return -EAGAIN in places where we've not actually done any work, like if > > > > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > > > > > even attempt the sf add). That kind of looks like a waste of transaction > > > > > rolls and further suggests it might be cleaner to break this whole path > > > > > down into helpers and put it back together in a way more conducive to > > > > > deferred operations. > > Er, agreed: > > > > > Yes, this area is a bit of a wart the way it is right now. I think you're > > > > right in that ultimately we may end up having to do a lot of refactoring in > > > > order to have more efficient "re-entry points". The state machine is hard > > > > to get into subroutines, so it's limited in use in the top level function. > > So my current understanding of the problem is that we have this big old > xfs_attr_set_args function that does multiple responsibilities requiring > transaction rolls, which we can't do directly inside a ->finish_item > handler: > > 1. If no attr fork, add one. > 2. If shortform attr fork, try to put it in the sf area. > 3. If shortform attr fork and out of space, convert to leaf format. > 4. Add attr to leaf/node attr tree. > And there are a bunch of tx rolls down in the #4 codepath that this series currently just tosses away. I'm not quite sure how appropriate that is, but I also don't think we necessarily need to preserve each and every transaction roll as implemented by the current code. IOW, I think it absolutely makes sense to step back from the current behavior and reassess the best/required places to roll xattr ops in progress as well as the transaction reservation itself. > So how about this: refactor each of these pieces into a separate > function, then add a separate XFS_ATTR_OP_FLAGS_* value for each of > these little pieces. xfs_trans_attr() can call the appropriate little > function for the OP_FLAG and xfs_attr_finish_item can figure out which > state comes next based on the return value. > > By directly mapping distinct OP_FLAGS to each piece of the attr setting > puzzle, you can use the existing "roll and come back" part of the defer > ops machinery. > > If _finish_item thinks we're done then we just exit. Otherwise, store > the new state in the (struct xfs_attr_item *) parameter passed into > _finish_item and return -EAGAIN, which puts the defer item back on the > defer op list, logs a new xattr intent with the new state, rolls the > transaction, and tries to finish the attr again. I think you've already > done this last part. > That sounds plausible to me. One concern I have is that I think we should try to avoid creating more unnecessary complexity in the dfops state mechanism simply to accommodate a messy xattr implementation. For example, consider the following sequence for a simple set of an xattr that requires leaf format and remote value block(s): - try sf add - returns -ENOSPC, convert to leaf and roll tx - attempt to add the xattr (xfs_attr_leaf_addname()) - if -ENOSPC, convert to node and call xfs_attr_node_addname() - else call xfs_attr3_leaf_add_work() - add entry - if remoteval, set INCOMPLETE - roll tx - if remoteval, call xfs_attr_rmtval_set() - block allocation, tx roll loop - copy remote value into bufs, xfs_bwrite() - if remoteval, xfs_attr3_leaf_clearflag() - clear INCOMPLETE - update/log rmt pointers - roll tx I'm wondering 1.) how much of this is necessary with an intent based implementation and 2.) how much of this can be refactored to not require complex state tracking. For example, all of the format conversions that occur before we actually make any modifications associated with the xattr (i.e., -ENOSPC returns from the current format) seem to me could easily be performed and immediately return -EAGAIN without any state tracking. The retry should pick up the current format of the fork and retry there. Thus, ISTM we could drop the whole xfs_attr_leaf_addname() -> xfs_attr3_leaf_to_node() -> xfs_attr_node_addname() codepath in favor of a format conversion and -EAGAIN retry that calls directly into xfs_attr_node_addname(). Once we have leaf format and we're doing remote block allocation, how much could we get away with by re-looking up the entry, finding that we're still short of remote blocks and performing another xfs_bmapi_write() -> -EAGAIN cycle until we're good to copy in the xattr value? What about all this INCOMPLETE stuff? Do we even need that with an intent based implementation? My understanding was that was because we had to roll the transaction and thus could leave an incomplete xattr on disk. I haven't looked too far into it so perhaps there's more to it than that, but if not and this is no longer a problem with an intent based implementation then perhaps much of that code and associated tx rolls can be bypassed as well. This is not to say that we won't require any such state tracking as you've described above. The whole block allocation thing above may require a state marker to get around attempts to set the xattr name again and get back to the remote value block allocation code. It also looks like we can do post xattr set format changes (i.e., node -> leaf, leaf -> sf) that might require something like that to make sure we don't go an retry an xattr set we've already completed. The point is just that I'd prefer that we explore how much we can simplify this mess of an implementation as much as possible (the above is all very handwavy) first to reduce the state tracking complexity, particularly if these states end up written to the log via the intent. Hmm, I'm starting to think that maybe what we really need to do here is step back from the code and logically map out what these states and the resulting operation flow needs to be, particularly since there are so many variations between different format conversions, renames, remote blocks, etc. Once we have this whole mess mapped out, coding it up should be more of an effort in refactoring. > xfs_attri_recover then becomes much simpler -- we're passed in the > reconstructed log item from which we figure out which step we need to > do. We call xfs_trans_attr() to do that one step, but unlike > _finish_item, we use the new state to construct a *new* attr intent and > attach it to the transaction, then call xfs_defer_move at the end to > move all the queued defer_ops to the parent_tp because log recovery > requires us to recover all the incomplete log intent items before > finishing any new ones that were created as part of recovery. > > This does mean that we end up with dramatically separate code paths for > defer ops attr setting vs. regular attr setting, but as you point out > the parent pointer feature will give the new code paths plenty of exercise. > Tying the new log intent items to a new feature bit is key to preventing > old kernels from stumbling across our new intent items, so we needed to > preserve the old attr set paths anyway. > That's a good point wrt to the other discussion around the direct xattr codepath. It sounds like we do need to keep that entire path around regardless to support v4 filesystems and such. The current series just unconditionally switches things over to deferred ops. > Anyway, if this all seems confusing, you can track me down, because I > wrote most of this system and therefore have forgotten all of > it^W^W^W^W^Wam available to help. :) > > > > > > > > > I was also starting to wonder if maybe I could do some refactoring in > > > > xfs_defer_finish_noroll to capture the common code associated with the > > > > -EAGAIN handling. Then maybe we could make a function pointer that we can > > > > pass through the finish_item interface. The idea being that subroutines > > > > could use the function pointer to cycle out the transaction when needed > > > > instead of having to record states and back out like this. It'd be a new > > The state tracking and rolling is already built into xfs_defer.c. :) > > > > > parameter to pipe around, but it'd be more efficient than the state machine, > > > > and less surgery in the refactor. And maybe a blessing to any other > > > > operations that might need to go through this transition in the future. > > > > Thoughts? > > > > > > > > > > That's an interesting idea. It still strikes me as a bit of a > > > fallback/hack as opposed to organizing the code to properly fit into the > > > dfops infrastructure, but it could be useful as a transient solution. > > > From a high level, it looks like we'd have to create a new intent, relog > > > this item and all remaining items associated with the dfp to it, roll > > > the tx, and finally create a done item associated with the intent in the > > > new tx. You'd need access to the dfp for some of that, so it's not > > > immediately clear to me that this ends up much easier than fixing up > > > the xattr code. > > (I think the code that handles EAGAIN being returned from finish_item > does this for you....) > Yeah, I'm not totally sure it's an ideal/feasible approach, but for the sake of clarity I think what Allison is getting at is that if there was a way to trigger a dfops -EAGAIN roll sequence via a callback/helper function, we wouldn't need to refactor the xattr subsystem to have -EAGAIN return points. Instead we could just invoke the callback at the existing roll points and achieve the same behavior (in theory). It's kind of like providing an inside-out xfs_defer_finish_noroll() -EAGAIN implementation via a helper function for code down in ->finish_item(). Brian > > > > > > BTW, if we did end up with something like that I'd probably prefer to > > > see it as an exported dfops helper function as opposed to a function > > > pointer being passed around, if possible. > > > > > > > Alrighty, I think for now I may try to pursue something more like what you > > proposed in the next patch and see where I get first. Maybe I'll come back > > to this later if for some reason it doesn't work out, but I think what you > > have there is reasonable. > > <nod> > > --D > > > > > Thanks again for the reviews! > > Allison > > > > > Brian > > > > > > > Thanks again for the reviews! > > > > > > > > Allison > > > > > > > > > > > > > > Brian > > > > > > > > > > > > > > > > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > > > > > > fs/xfs/scrub/common.c | 2 ++ > > > > > > fs/xfs/xfs_acl.c | 2 ++ > > > > > > fs/xfs/xfs_attr_item.c | 2 +- > > > > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > > > > fs/xfs/xfs_ioctl32.c | 2 ++ > > > > > > fs/xfs/xfs_iops.c | 1 + > > > > > > fs/xfs/xfs_xattr.c | 1 + > > > > > > 8 files changed, 28 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > > index 974c963..4ce3b0a 100644 > > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > > > > > > char a_name[1]; /* attr name (NULL terminated) */ > > > > > > } attrlist_ent_t; > > > > > > +/* Attr state machine types */ > > > > > > +enum xfs_attr_state { > > > > > > + XFS_ATTR_STATE1 = 1, > > > > > > + XFS_ATTR_STATE2 = 2, > > > > > > + XFS_ATTR_STATE3 = 3, > > > > > > +}; > > > > > > + > > > > > > /* > > > > > > * List of attrs to commit later. > > > > > > */ > > > > > > @@ -88,7 +95,16 @@ struct xfs_attr_item { > > > > > > void *xattri_name; /* attr name */ > > > > > > uint32_t xattri_name_len; /* length of name */ > > > > > > uint32_t xattri_flags; /* attr flags */ > > > > > > - struct list_head xattri_list; > > > > > > + > > > > > > + /* > > > > > > + * Delayed attr parameters that need to remain instantiated > > > > > > + * across transaction rolls during the defer finish > > > > > > + */ > > > > > > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > > > > > > + enum xfs_attr_state xattri_state; /* state machine marker */ > > > > > > + struct xfs_da_args xattri_args; /* args context */ > > > > > > + > > > > > > + struct list_head xattri_list; > > > > > > /* > > > > > > * A byte array follows the header containing the file name and > > > > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > > > > > > index 0c54ff5..270c32e 100644 > > > > > > --- a/fs/xfs/scrub/common.c > > > > > > +++ b/fs/xfs/scrub/common.c > > > > > > @@ -30,6 +30,8 @@ > > > > > > #include "xfs_rmap_btree.h" > > > > > > #include "xfs_log.h" > > > > > > #include "xfs_trans_priv.h" > > > > > > +#include "xfs_da_format.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_reflink.h" > > > > > > #include "scrub/xfs_scrub.h" > > > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > > > > > index 142de8d..9b1b93e 100644 > > > > > > --- a/fs/xfs/xfs_acl.c > > > > > > +++ b/fs/xfs/xfs_acl.c > > > > > > @@ -10,6 +10,8 @@ > > > > > > #include "xfs_mount.h" > > > > > > #include "xfs_inode.h" > > > > > > #include "xfs_acl.h" > > > > > > +#include "xfs_da_format.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_trace.h" > > > > > > #include <linux/slab.h> > > > > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > > > > > index 0ea19b4..36e6d1e 100644 > > > > > > --- a/fs/xfs/xfs_attr_item.c > > > > > > +++ b/fs/xfs/xfs_attr_item.c > > > > > > @@ -19,10 +19,10 @@ > > > > > > #include "xfs_rmap.h" > > > > > > #include "xfs_inode.h" > > > > > > #include "xfs_icache.h" > > > > > > -#include "xfs_attr.h" > > > > > > #include "xfs_shared.h" > > > > > > #include "xfs_da_format.h" > > > > > > #include "xfs_da_btree.h" > > > > > > +#include "xfs_attr.h" > > > > > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > > > > > > { > > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > > > > index ab341d6..c8728ca 100644 > > > > > > --- a/fs/xfs/xfs_ioctl.c > > > > > > +++ b/fs/xfs/xfs_ioctl.c > > > > > > @@ -16,6 +16,8 @@ > > > > > > #include "xfs_rtalloc.h" > > > > > > #include "xfs_itable.h" > > > > > > #include "xfs_error.h" > > > > > > +#include "xfs_da_format.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_bmap.h" > > > > > > #include "xfs_bmap_util.h" > > > > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > > > > > > index 5001dca..23f6990 100644 > > > > > > --- a/fs/xfs/xfs_ioctl32.c > > > > > > +++ b/fs/xfs/xfs_ioctl32.c > > > > > > @@ -21,6 +21,8 @@ > > > > > > #include "xfs_fsops.h" > > > > > > #include "xfs_alloc.h" > > > > > > #include "xfs_rtalloc.h" > > > > > > +#include "xfs_da_format.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_ioctl.h" > > > > > > #include "xfs_ioctl32.h" > > > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > > > > index e73c21a..561c467 100644 > > > > > > --- a/fs/xfs/xfs_iops.c > > > > > > +++ b/fs/xfs/xfs_iops.c > > > > > > @@ -17,6 +17,7 @@ > > > > > > #include "xfs_acl.h" > > > > > > #include "xfs_quota.h" > > > > > > #include "xfs_error.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_trans.h" > > > > > > #include "xfs_trace.h" > > > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > > > > > index 3013746..938e81d 100644 > > > > > > --- a/fs/xfs/xfs_xattr.c > > > > > > +++ b/fs/xfs/xfs_xattr.c > > > > > > @@ -11,6 +11,7 @@ > > > > > > #include "xfs_mount.h" > > > > > > #include "xfs_da_format.h" > > > > > > #include "xfs_inode.h" > > > > > > +#include "xfs_da_btree.h" > > > > > > #include "xfs_attr.h" > > > > > > #include "xfs_attr_leaf.h" > > > > > > #include "xfs_acl.h" > > > > > > -- > > > > > > 2.7.4 > > > > > >
On Wed, Apr 24, 2019 at 08:17:48AM -0400, Brian Foster wrote: > On Tue, Apr 23, 2019 at 09:10:16PM -0700, Darrick J. Wong wrote: > > Sorry I'm late back to the party... > > > > On Tue, Apr 23, 2019 at 07:24:40PM -0700, Allison Henderson wrote: > > > > > > On 4/23/19 6:20 AM, Brian Foster wrote: > > > > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: > > > > > > > > > > > > > > > On 4/22/19 6:03 AM, Brian Foster wrote: > > > > > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > > > > > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > > > > > > > and a new state type. We will use these in the next patch when > > > > > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > > > > > > > Because the subroutines of this function modify the contents of these > > > > > > > structures, we need to find a place to store them where they remain > > > > > > > instantiated across multiple calls to xfs_set_attr_args. > > > > > > > > > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > > > > --- > > > > > > > > > > > > I see Darrick has already commented on the whole state thing. I'll > > > > > > probably have to grok the next patch to comment further, but just a > > > > > > couple initial thoughts: > > > > > > > > > > > > First, I hit a build failure with this patch. It looks like there's a > > > > > > missed include in the scrub code: > > > > > > > > > > > > ... > > > > > > CC [M] fs/xfs/scrub/repair.o > > > > > > In file included from fs/xfs/scrub/repair.c:32: > > > > > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > > > > > > struct xfs_da_args xattri_args; /* args context */ > > > > > Hmm, ok. I'll get that corrected, I probably need to clean out my workspace > > > > > and build from scratch. > > > > > > > > > > > ... > > > > > > > > > > > > Second, the commit log suggests that the states will reflect the current > > > > > > transaction roll points (i.e., establishing re-entry points down in > > > > > > xfs_attr_set_args(). I'm kind of wondering if we should break these > > > > > > xattr set sub-sequences down into smaller helper functions (refactoring > > > > > > the existing code as we go) such that the mechanism could technically be > > > > I had had the thought of "why not just give each step of setting an > > attribute its own log item, so we don't have to have this STATE_NNN > > business?" but then realized that will generate an insane amount of > > boilerplate, and you're already close to a better solution, so I shut up > > to think harder. :) > > > > The thought of separating things down into smaller "ops" popped into my > head (not necessarily separate/smaller log items), but I hadn't really > thought it through to this point... > > > > > > > used deferred or not. Re: the previous thought on whether to defer xattr > > > > > > removes or not, there might also be cases where there's not a need to > > > > > > defer xattr sets. > > > > > > > > > > > > E.g., taking a quick peek into the next patch, the state 1 case in > > > > > > xfs_attr_try_sf_addname() is actually a transaction commit, which I > > > > > > think means we're done. We'd have done an attr memory allocation, > > > > > > deferred op and transaction roll where none was necessary so it might > > > > > > not be worth it to defer in that scenario. Hmm, it also looks like we > > > > > > return -EAGAIN in places where we've not actually done any work, like if > > > > > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > > > > > > even attempt the sf add). That kind of looks like a waste of transaction > > > > > > rolls and further suggests it might be cleaner to break this whole path > > > > > > down into helpers and put it back together in a way more conducive to > > > > > > deferred operations. > > > > Er, agreed: > > > > > > > Yes, this area is a bit of a wart the way it is right now. I think you're > > > > > right in that ultimately we may end up having to do a lot of refactoring in > > > > > order to have more efficient "re-entry points". The state machine is hard > > > > > to get into subroutines, so it's limited in use in the top level function. > > > > So my current understanding of the problem is that we have this big old > > xfs_attr_set_args function that does multiple responsibilities requiring > > transaction rolls, which we can't do directly inside a ->finish_item > > handler: > > > > 1. If no attr fork, add one. > > 2. If shortform attr fork, try to put it in the sf area. > > 3. If shortform attr fork and out of space, convert to leaf format. > > 4. Add attr to leaf/node attr tree. > > > > And there are a bunch of tx rolls down in the #4 codepath that this > series currently just tosses away. I'm not quite sure how appropriate > that is, but I also don't think we necessarily need to preserve each and > every transaction roll as implemented by the current code. > > IOW, I think it absolutely makes sense to step back from the current > behavior and reassess the best/required places to roll xattr ops in > progress as well as the transaction reservation itself. Yes, it would help to make a list of every small step that could possibly be required to set an attribute. That will help narrow down how many defer op pieces are needed. Another thought I had is that having the finish_item continually logging a new intent with the latest state means that we can free the old intent item, which helps us avoid the problem of pinning the log tail at that first intent item while we scramble around doing a whole lot of rolling and other work to get to the done item. > > So how about this: refactor each of these pieces into a separate > > function, then add a separate XFS_ATTR_OP_FLAGS_* value for each of > > these little pieces. xfs_trans_attr() can call the appropriate little > > function for the OP_FLAG and xfs_attr_finish_item can figure out which > > state comes next based on the return value. > > > > By directly mapping distinct OP_FLAGS to each piece of the attr setting > > puzzle, you can use the existing "roll and come back" part of the defer > > ops machinery. > > > > If _finish_item thinks we're done then we just exit. Otherwise, store > > the new state in the (struct xfs_attr_item *) parameter passed into > > _finish_item and return -EAGAIN, which puts the defer item back on the > > defer op list, logs a new xattr intent with the new state, rolls the > > transaction, and tries to finish the attr again. I think you've already > > done this last part. > > > > That sounds plausible to me. One concern I have is that I think we > should try to avoid creating more unnecessary complexity in the dfops > state mechanism simply to accommodate a messy xattr implementation. For > example, consider the following sequence for a simple set of an xattr > that requires leaf format and remote value block(s): > > - try sf add > - returns -ENOSPC, convert to leaf and roll tx > - attempt to add the xattr (xfs_attr_leaf_addname()) > - if -ENOSPC, convert to node and call xfs_attr_node_addname() > - else call xfs_attr3_leaf_add_work() > - add entry > - if remoteval, set INCOMPLETE > - roll tx > - if remoteval, call xfs_attr_rmtval_set() > - block allocation, tx roll loop > - copy remote value into bufs, xfs_bwrite() > - if remoteval, xfs_attr3_leaf_clearflag() > - clear INCOMPLETE > - update/log rmt pointers > - roll tx > > I'm wondering 1.) how much of this is necessary with an intent based > implementation and 2.) how much of this can be refactored to not require > complex state tracking. > > For example, all of the format conversions that occur before we actually > make any modifications associated with the xattr (i.e., -ENOSPC returns > from the current format) seem to me could easily be performed and > immediately return -EAGAIN without any state tracking. The retry should > pick up the current format of the fork and retry there. Thus, ISTM we > could drop the whole xfs_attr_leaf_addname() -> xfs_attr3_leaf_to_node() > -> xfs_attr_node_addname() codepath in favor of a format conversion and > -EAGAIN retry that calls directly into xfs_attr_node_addname(). That had been my other thought -- in theory we keep the inode locked across all the transaction rolls, so we could auto-detect what we need to do. > Once we have leaf format and we're doing remote block allocation, how > much could we get away with by re-looking up the entry, finding that > we're still short of remote blocks and performing another > xfs_bmapi_write() -> -EAGAIN cycle until we're good to copy in the xattr > value? > > What about all this INCOMPLETE stuff? Do we even need that with an > intent based implementation? No. AFAIK the INCOMPLETE flag exists to hide attrs from userspace until we're totally done setting them up, and is therefore unnecessary with an intent implementation. Repair zaps any INCOMPLETE attrs it finds. > My understanding was that was because we > had to roll the transaction and thus could leave an incomplete xattr on > disk. I haven't looked too far into it so perhaps there's more to it > than that, but if not and this is no longer a problem with an intent > based implementation then perhaps much of that code and associated tx > rolls can be bypassed as well. Getting rid of the INCOMPLETE wonkiness would be the strongest argument for switching the regular attr manipulation paths to use intents, though we'd have to toggle it with some feature or other. (Some feature or other being parent pointers, or possibly just migrating the free space tracking parts of dir3 to a "new" attr4 format for better speed.) > This is not to say that we won't require any such state tracking as > you've described above. The whole block allocation thing above may > require a state marker to get around attempts to set the xattr name > again and get back to the remote value block allocation code. It also > looks like we can do post xattr set format changes (i.e., node -> leaf, > leaf -> sf) that might require something like that to make sure we don't > go an retry an xattr set we've already completed. The point is just that > I'd prefer that we explore how much we can simplify this mess of an > implementation as much as possible (the above is all very handwavy) > first to reduce the state tracking complexity, particularly if these > states end up written to the log via the intent. > > Hmm, I'm starting to think that maybe what we really need to do here is > step back from the code and logically map out what these states and the > resulting operation flow needs to be, particularly since there are so > many variations between different format conversions, renames, remote > blocks, etc. Once we have this whole mess mapped out, coding it up > should be more of an effort in refactoring. Yep. > > xfs_attri_recover then becomes much simpler -- we're passed in the > > reconstructed log item from which we figure out which step we need to > > do. We call xfs_trans_attr() to do that one step, but unlike > > _finish_item, we use the new state to construct a *new* attr intent and > > attach it to the transaction, then call xfs_defer_move at the end to > > move all the queued defer_ops to the parent_tp because log recovery > > requires us to recover all the incomplete log intent items before > > finishing any new ones that were created as part of recovery. > > > > This does mean that we end up with dramatically separate code paths for > > defer ops attr setting vs. regular attr setting, but as you point out > > the parent pointer feature will give the new code paths plenty of exercise. > > Tying the new log intent items to a new feature bit is key to preventing > > old kernels from stumbling across our new intent items, so we needed to > > preserve the old attr set paths anyway. > > > > That's a good point wrt to the other discussion around the direct xattr > codepath. It sounds like we do need to keep that entire path around > regardless to support v4 filesystems and such. The current series just > unconditionally switches things over to deferred ops. Er... yikes. XFS cannot suddenly introduce new ondisk formats for existing filesystems. > > Anyway, if this all seems confusing, you can track me down, because I > > wrote most of this system and therefore have forgotten all of > > it^W^W^W^W^Wam available to help. :) > > > > > > > > > > > > I was also starting to wonder if maybe I could do some refactoring in > > > > > xfs_defer_finish_noroll to capture the common code associated with the > > > > > -EAGAIN handling. Then maybe we could make a function pointer that we can > > > > > pass through the finish_item interface. The idea being that subroutines > > > > > could use the function pointer to cycle out the transaction when needed > > > > > instead of having to record states and back out like this. It'd be a new > > > > The state tracking and rolling is already built into xfs_defer.c. :) > > > > > > > parameter to pipe around, but it'd be more efficient than the state machine, > > > > > and less surgery in the refactor. And maybe a blessing to any other > > > > > operations that might need to go through this transition in the future. > > > > > Thoughts? > > > > > > > > > > > > > That's an interesting idea. It still strikes me as a bit of a > > > > fallback/hack as opposed to organizing the code to properly fit into the > > > > dfops infrastructure, but it could be useful as a transient solution. > > > > From a high level, it looks like we'd have to create a new intent, relog > > > > this item and all remaining items associated with the dfp to it, roll > > > > the tx, and finally create a done item associated with the intent in the > > > > new tx. You'd need access to the dfp for some of that, so it's not > > > > immediately clear to me that this ends up much easier than fixing up > > > > the xattr code. > > > > (I think the code that handles EAGAIN being returned from finish_item > > does this for you....) > > > > Yeah, I'm not totally sure it's an ideal/feasible approach, but for the > sake of clarity I think what Allison is getting at is that if there was > a way to trigger a dfops -EAGAIN roll sequence via a callback/helper > function, we wouldn't need to refactor the xattr subsystem to have > -EAGAIN return points. Instead we could just invoke the callback at the > existing roll points and achieve the same behavior (in theory). It's > kind of like providing an inside-out xfs_defer_finish_noroll() -EAGAIN > implementation via a helper function for code down in ->finish_item(). <nod> I grok that, but wonder if we really can invoke a roll while in the middle of ->finish_item...? Anyway, we can set aside my confusion for now because I really think we need to see a map of all the pieces --D > Brian > > > > > > > > > BTW, if we did end up with something like that I'd probably prefer to > > > > see it as an exported dfops helper function as opposed to a function > > > > pointer being passed around, if possible. > > > > > > > > > > Alrighty, I think for now I may try to pursue something more like what you > > > proposed in the next patch and see where I get first. Maybe I'll come back > > > to this later if for some reason it doesn't work out, but I think what you > > > have there is reasonable. > > > > <nod> > > > > --D > > > > > > > > Thanks again for the reviews! > > > Allison > > > > > > > Brian > > > > > > > > > Thanks again for the reviews! > > > > > > > > > > Allison > > > > > > > > > > > > > > > > > Brian > > > > > > > > > > > > > > > > > > > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > > > > > > > fs/xfs/scrub/common.c | 2 ++ > > > > > > > fs/xfs/xfs_acl.c | 2 ++ > > > > > > > fs/xfs/xfs_attr_item.c | 2 +- > > > > > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > > > > > fs/xfs/xfs_ioctl32.c | 2 ++ > > > > > > > fs/xfs/xfs_iops.c | 1 + > > > > > > > fs/xfs/xfs_xattr.c | 1 + > > > > > > > 8 files changed, 28 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > > > index 974c963..4ce3b0a 100644 > > > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > > > > > > > char a_name[1]; /* attr name (NULL terminated) */ > > > > > > > } attrlist_ent_t; > > > > > > > +/* Attr state machine types */ > > > > > > > +enum xfs_attr_state { > > > > > > > + XFS_ATTR_STATE1 = 1, > > > > > > > + XFS_ATTR_STATE2 = 2, > > > > > > > + XFS_ATTR_STATE3 = 3, > > > > > > > +}; > > > > > > > + > > > > > > > /* > > > > > > > * List of attrs to commit later. > > > > > > > */ > > > > > > > @@ -88,7 +95,16 @@ struct xfs_attr_item { > > > > > > > void *xattri_name; /* attr name */ > > > > > > > uint32_t xattri_name_len; /* length of name */ > > > > > > > uint32_t xattri_flags; /* attr flags */ > > > > > > > - struct list_head xattri_list; > > > > > > > + > > > > > > > + /* > > > > > > > + * Delayed attr parameters that need to remain instantiated > > > > > > > + * across transaction rolls during the defer finish > > > > > > > + */ > > > > > > > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > > > > > > > + enum xfs_attr_state xattri_state; /* state machine marker */ > > > > > > > + struct xfs_da_args xattri_args; /* args context */ > > > > > > > + > > > > > > > + struct list_head xattri_list; > > > > > > > /* > > > > > > > * A byte array follows the header containing the file name and > > > > > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > > > > > > > index 0c54ff5..270c32e 100644 > > > > > > > --- a/fs/xfs/scrub/common.c > > > > > > > +++ b/fs/xfs/scrub/common.c > > > > > > > @@ -30,6 +30,8 @@ > > > > > > > #include "xfs_rmap_btree.h" > > > > > > > #include "xfs_log.h" > > > > > > > #include "xfs_trans_priv.h" > > > > > > > +#include "xfs_da_format.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_reflink.h" > > > > > > > #include "scrub/xfs_scrub.h" > > > > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > > > > > > index 142de8d..9b1b93e 100644 > > > > > > > --- a/fs/xfs/xfs_acl.c > > > > > > > +++ b/fs/xfs/xfs_acl.c > > > > > > > @@ -10,6 +10,8 @@ > > > > > > > #include "xfs_mount.h" > > > > > > > #include "xfs_inode.h" > > > > > > > #include "xfs_acl.h" > > > > > > > +#include "xfs_da_format.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_trace.h" > > > > > > > #include <linux/slab.h> > > > > > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > > > > > > index 0ea19b4..36e6d1e 100644 > > > > > > > --- a/fs/xfs/xfs_attr_item.c > > > > > > > +++ b/fs/xfs/xfs_attr_item.c > > > > > > > @@ -19,10 +19,10 @@ > > > > > > > #include "xfs_rmap.h" > > > > > > > #include "xfs_inode.h" > > > > > > > #include "xfs_icache.h" > > > > > > > -#include "xfs_attr.h" > > > > > > > #include "xfs_shared.h" > > > > > > > #include "xfs_da_format.h" > > > > > > > #include "xfs_da_btree.h" > > > > > > > +#include "xfs_attr.h" > > > > > > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > > > > > > > { > > > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > > > > > index ab341d6..c8728ca 100644 > > > > > > > --- a/fs/xfs/xfs_ioctl.c > > > > > > > +++ b/fs/xfs/xfs_ioctl.c > > > > > > > @@ -16,6 +16,8 @@ > > > > > > > #include "xfs_rtalloc.h" > > > > > > > #include "xfs_itable.h" > > > > > > > #include "xfs_error.h" > > > > > > > +#include "xfs_da_format.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_bmap.h" > > > > > > > #include "xfs_bmap_util.h" > > > > > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > > > > > > > index 5001dca..23f6990 100644 > > > > > > > --- a/fs/xfs/xfs_ioctl32.c > > > > > > > +++ b/fs/xfs/xfs_ioctl32.c > > > > > > > @@ -21,6 +21,8 @@ > > > > > > > #include "xfs_fsops.h" > > > > > > > #include "xfs_alloc.h" > > > > > > > #include "xfs_rtalloc.h" > > > > > > > +#include "xfs_da_format.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_ioctl.h" > > > > > > > #include "xfs_ioctl32.h" > > > > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > > > > > index e73c21a..561c467 100644 > > > > > > > --- a/fs/xfs/xfs_iops.c > > > > > > > +++ b/fs/xfs/xfs_iops.c > > > > > > > @@ -17,6 +17,7 @@ > > > > > > > #include "xfs_acl.h" > > > > > > > #include "xfs_quota.h" > > > > > > > #include "xfs_error.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_trans.h" > > > > > > > #include "xfs_trace.h" > > > > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > > > > > > index 3013746..938e81d 100644 > > > > > > > --- a/fs/xfs/xfs_xattr.c > > > > > > > +++ b/fs/xfs/xfs_xattr.c > > > > > > > @@ -11,6 +11,7 @@ > > > > > > > #include "xfs_mount.h" > > > > > > > #include "xfs_da_format.h" > > > > > > > #include "xfs_inode.h" > > > > > > > +#include "xfs_da_btree.h" > > > > > > > #include "xfs_attr.h" > > > > > > > #include "xfs_attr_leaf.h" > > > > > > > #include "xfs_acl.h" > > > > > > > -- > > > > > > > 2.7.4 > > > > > > >
On Wed, Apr 24, 2019 at 08:25:33AM -0700, Darrick J. Wong wrote: > On Wed, Apr 24, 2019 at 08:17:48AM -0400, Brian Foster wrote: > > On Tue, Apr 23, 2019 at 09:10:16PM -0700, Darrick J. Wong wrote: > > > Sorry I'm late back to the party... > > > > > > On Tue, Apr 23, 2019 at 07:24:40PM -0700, Allison Henderson wrote: > > > > > > > > On 4/23/19 6:20 AM, Brian Foster wrote: > > > > > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote: > > > > > > > > > > > > > > > > > > On 4/22/19 6:03 AM, Brian Foster wrote: > > > > > > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote: > > > > > > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer > > > > > > > > and a new state type. We will use these in the next patch when > > > > > > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN. > > > > > > > > Because the subroutines of this function modify the contents of these > > > > > > > > structures, we need to find a place to store them where they remain > > > > > > > > instantiated across multiple calls to xfs_set_attr_args. > > > > > > > > > > > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > > > > > > > --- > > > > > > > > > > > > > > I see Darrick has already commented on the whole state thing. I'll > > > > > > > probably have to grok the next patch to comment further, but just a > > > > > > > couple initial thoughts: > > > > > > > > > > > > > > First, I hit a build failure with this patch. It looks like there's a > > > > > > > missed include in the scrub code: > > > > > > > > > > > > > > ... > > > > > > > CC [M] fs/xfs/scrub/repair.o > > > > > > > In file included from fs/xfs/scrub/repair.c:32: > > > > > > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type > > > > > > > struct xfs_da_args xattri_args; /* args context */ > > > > > > Hmm, ok. I'll get that corrected, I probably need to clean out my workspace > > > > > > and build from scratch. > > > > > > > > > > > > > ... > > > > > > > > > > > > > > Second, the commit log suggests that the states will reflect the current > > > > > > > transaction roll points (i.e., establishing re-entry points down in > > > > > > > xfs_attr_set_args(). I'm kind of wondering if we should break these > > > > > > > xattr set sub-sequences down into smaller helper functions (refactoring > > > > > > > the existing code as we go) such that the mechanism could technically be > > > > > > I had had the thought of "why not just give each step of setting an > > > attribute its own log item, so we don't have to have this STATE_NNN > > > business?" but then realized that will generate an insane amount of > > > boilerplate, and you're already close to a better solution, so I shut up > > > to think harder. :) > > > > > > > The thought of separating things down into smaller "ops" popped into my > > head (not necessarily separate/smaller log items), but I hadn't really > > thought it through to this point... > > > > > > > > > used deferred or not. Re: the previous thought on whether to defer xattr > > > > > > > removes or not, there might also be cases where there's not a need to > > > > > > > defer xattr sets. > > > > > > > > > > > > > > E.g., taking a quick peek into the next patch, the state 1 case in > > > > > > > xfs_attr_try_sf_addname() is actually a transaction commit, which I > > > > > > > think means we're done. We'd have done an attr memory allocation, > > > > > > > deferred op and transaction roll where none was necessary so it might > > > > > > > not be worth it to defer in that scenario. Hmm, it also looks like we > > > > > > > return -EAGAIN in places where we've not actually done any work, like if > > > > > > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we > > > > > > > even attempt the sf add). That kind of looks like a waste of transaction > > > > > > > rolls and further suggests it might be cleaner to break this whole path > > > > > > > down into helpers and put it back together in a way more conducive to > > > > > > > deferred operations. > > > > > > Er, agreed: > > > > > > > > > Yes, this area is a bit of a wart the way it is right now. I think you're > > > > > > right in that ultimately we may end up having to do a lot of refactoring in > > > > > > order to have more efficient "re-entry points". The state machine is hard > > > > > > to get into subroutines, so it's limited in use in the top level function. > > > > > > So my current understanding of the problem is that we have this big old > > > xfs_attr_set_args function that does multiple responsibilities requiring > > > transaction rolls, which we can't do directly inside a ->finish_item > > > handler: > > > > > > 1. If no attr fork, add one. > > > 2. If shortform attr fork, try to put it in the sf area. > > > 3. If shortform attr fork and out of space, convert to leaf format. > > > 4. Add attr to leaf/node attr tree. > > > > > > > And there are a bunch of tx rolls down in the #4 codepath that this > > series currently just tosses away. I'm not quite sure how appropriate > > that is, but I also don't think we necessarily need to preserve each and > > every transaction roll as implemented by the current code. > > > > IOW, I think it absolutely makes sense to step back from the current > > behavior and reassess the best/required places to roll xattr ops in > > progress as well as the transaction reservation itself. > > Yes, it would help to make a list of every small step that could > possibly be required to set an attribute. That will help narrow down > how many defer op pieces are needed. > > Another thought I had is that having the finish_item continually logging > a new intent with the latest state means that we can free the old intent > item, which helps us avoid the problem of pinning the log tail at that > first intent item while we scramble around doing a whole lot of rolling > and other work to get to the done item. > > > > So how about this: refactor each of these pieces into a separate > > > function, then add a separate XFS_ATTR_OP_FLAGS_* value for each of > > > these little pieces. xfs_trans_attr() can call the appropriate little > > > function for the OP_FLAG and xfs_attr_finish_item can figure out which > > > state comes next based on the return value. > > > > > > By directly mapping distinct OP_FLAGS to each piece of the attr setting > > > puzzle, you can use the existing "roll and come back" part of the defer > > > ops machinery. > > > > > > If _finish_item thinks we're done then we just exit. Otherwise, store > > > the new state in the (struct xfs_attr_item *) parameter passed into > > > _finish_item and return -EAGAIN, which puts the defer item back on the > > > defer op list, logs a new xattr intent with the new state, rolls the > > > transaction, and tries to finish the attr again. I think you've already > > > done this last part. > > > > > > > That sounds plausible to me. One concern I have is that I think we > > should try to avoid creating more unnecessary complexity in the dfops > > state mechanism simply to accommodate a messy xattr implementation. For > > example, consider the following sequence for a simple set of an xattr > > that requires leaf format and remote value block(s): > > > > - try sf add > > - returns -ENOSPC, convert to leaf and roll tx > > - attempt to add the xattr (xfs_attr_leaf_addname()) > > - if -ENOSPC, convert to node and call xfs_attr_node_addname() > > - else call xfs_attr3_leaf_add_work() > > - add entry > > - if remoteval, set INCOMPLETE > > - roll tx > > - if remoteval, call xfs_attr_rmtval_set() > > - block allocation, tx roll loop > > - copy remote value into bufs, xfs_bwrite() > > - if remoteval, xfs_attr3_leaf_clearflag() > > - clear INCOMPLETE > > - update/log rmt pointers > > - roll tx > > > > I'm wondering 1.) how much of this is necessary with an intent based > > implementation and 2.) how much of this can be refactored to not require > > complex state tracking. > > > > For example, all of the format conversions that occur before we actually > > make any modifications associated with the xattr (i.e., -ENOSPC returns > > from the current format) seem to me could easily be performed and > > immediately return -EAGAIN without any state tracking. The retry should > > pick up the current format of the fork and retry there. Thus, ISTM we > > could drop the whole xfs_attr_leaf_addname() -> xfs_attr3_leaf_to_node() > > -> xfs_attr_node_addname() codepath in favor of a format conversion and > > -EAGAIN retry that calls directly into xfs_attr_node_addname(). > > That had been my other thought -- in theory we keep the inode locked > across all the transaction rolls, so we could auto-detect what we need > to do. > Indeed, at the very least it might reduce the number of "on-disk" state markers we have to define. > > Once we have leaf format and we're doing remote block allocation, how > > much could we get away with by re-looking up the entry, finding that > > we're still short of remote blocks and performing another > > xfs_bmapi_write() -> -EAGAIN cycle until we're good to copy in the xattr > > value? > > > > What about all this INCOMPLETE stuff? Do we even need that with an > > intent based implementation? > > No. AFAIK the INCOMPLETE flag exists to hide attrs from userspace until > we're totally done setting them up, and is therefore unnecessary with an > intent implementation. Repair zaps any INCOMPLETE attrs it finds. > > > My understanding was that was because we > > had to roll the transaction and thus could leave an incomplete xattr on > > disk. I haven't looked too far into it so perhaps there's more to it > > than that, but if not and this is no longer a problem with an intent > > based implementation then perhaps much of that code and associated tx > > rolls can be bypassed as well. > > Getting rid of the INCOMPLETE wonkiness would be the strongest argument > for switching the regular attr manipulation paths to use intents, though > we'd have to toggle it with some feature or other. > > (Some feature or other being parent pointers, or possibly just migrating > the free space tracking parts of dir3 to a "new" attr4 format for better > speed.) > > > This is not to say that we won't require any such state tracking as > > you've described above. The whole block allocation thing above may > > require a state marker to get around attempts to set the xattr name > > again and get back to the remote value block allocation code. It also > > looks like we can do post xattr set format changes (i.e., node -> leaf, > > leaf -> sf) that might require something like that to make sure we don't > > go an retry an xattr set we've already completed. The point is just that > > I'd prefer that we explore how much we can simplify this mess of an > > implementation as much as possible (the above is all very handwavy) > > first to reduce the state tracking complexity, particularly if these > > states end up written to the log via the intent. > > > > Hmm, I'm starting to think that maybe what we really need to do here is > > step back from the code and logically map out what these states and the > > resulting operation flow needs to be, particularly since there are so > > many variations between different format conversions, renames, remote > > blocks, etc. Once we have this whole mess mapped out, coding it up > > should be more of an effort in refactoring. > > Yep. > > > > xfs_attri_recover then becomes much simpler -- we're passed in the > > > reconstructed log item from which we figure out which step we need to > > > do. We call xfs_trans_attr() to do that one step, but unlike > > > _finish_item, we use the new state to construct a *new* attr intent and > > > attach it to the transaction, then call xfs_defer_move at the end to > > > move all the queued defer_ops to the parent_tp because log recovery > > > requires us to recover all the incomplete log intent items before > > > finishing any new ones that were created as part of recovery. > > > > > > This does mean that we end up with dramatically separate code paths for > > > defer ops attr setting vs. regular attr setting, but as you point out > > > the parent pointer feature will give the new code paths plenty of exercise. > > > Tying the new log intent items to a new feature bit is key to preventing > > > old kernels from stumbling across our new intent items, so we needed to > > > preserve the old attr set paths anyway. > > > > > > > That's a good point wrt to the other discussion around the direct xattr > > codepath. It sounds like we do need to keep that entire path around > > regardless to support v4 filesystems and such. The current series just > > unconditionally switches things over to deferred ops. > > Er... yikes. XFS cannot suddenly introduce new ondisk formats for > existing filesystems. > We were discussing whether to preserve the existing codepath with respect to flexibility/efficiency, but the whole backwards compatibility aspect just didn't register with me until you mentioned it. I think that kind of makes that decision for us. :P > > > Anyway, if this all seems confusing, you can track me down, because I > > > wrote most of this system and therefore have forgotten all of > > > it^W^W^W^W^Wam available to help. :) > > > > > > > > > > > > > > > I was also starting to wonder if maybe I could do some refactoring in > > > > > > xfs_defer_finish_noroll to capture the common code associated with the > > > > > > -EAGAIN handling. Then maybe we could make a function pointer that we can > > > > > > pass through the finish_item interface. The idea being that subroutines > > > > > > could use the function pointer to cycle out the transaction when needed > > > > > > instead of having to record states and back out like this. It'd be a new > > > > > > The state tracking and rolling is already built into xfs_defer.c. :) > > > > > > > > > parameter to pipe around, but it'd be more efficient than the state machine, > > > > > > and less surgery in the refactor. And maybe a blessing to any other > > > > > > operations that might need to go through this transition in the future. > > > > > > Thoughts? > > > > > > > > > > > > > > > > That's an interesting idea. It still strikes me as a bit of a > > > > > fallback/hack as opposed to organizing the code to properly fit into the > > > > > dfops infrastructure, but it could be useful as a transient solution. > > > > > From a high level, it looks like we'd have to create a new intent, relog > > > > > this item and all remaining items associated with the dfp to it, roll > > > > > the tx, and finally create a done item associated with the intent in the > > > > > new tx. You'd need access to the dfp for some of that, so it's not > > > > > immediately clear to me that this ends up much easier than fixing up > > > > > the xattr code. > > > > > > (I think the code that handles EAGAIN being returned from finish_item > > > does this for you....) > > > > > > > Yeah, I'm not totally sure it's an ideal/feasible approach, but for the > > sake of clarity I think what Allison is getting at is that if there was > > a way to trigger a dfops -EAGAIN roll sequence via a callback/helper > > function, we wouldn't need to refactor the xattr subsystem to have > > -EAGAIN return points. Instead we could just invoke the callback at the > > existing roll points and achieve the same behavior (in theory). It's > > kind of like providing an inside-out xfs_defer_finish_noroll() -EAGAIN > > implementation via a helper function for code down in ->finish_item(). > > <nod> I grok that, but wonder if we really can invoke a roll while in > the middle of ->finish_item...? Anyway, we can set aside my confusion > for now because I really think we need to see a map of all the pieces > Ok, I'm not really sure either. It was just an idea to bat around with the rest. I agree that an informal, logical map/breakdown is the best next step here. That gives us something concrete to review and refine. Brian > --D > > > Brian > > > > > > > > > > > > BTW, if we did end up with something like that I'd probably prefer to > > > > > see it as an exported dfops helper function as opposed to a function > > > > > pointer being passed around, if possible. > > > > > > > > > > > > > Alrighty, I think for now I may try to pursue something more like what you > > > > proposed in the next patch and see where I get first. Maybe I'll come back > > > > to this later if for some reason it doesn't work out, but I think what you > > > > have there is reasonable. > > > > > > <nod> > > > > > > --D > > > > > > > > > > > Thanks again for the reviews! > > > > Allison > > > > > > > > > Brian > > > > > > > > > > > Thanks again for the reviews! > > > > > > > > > > > > Allison > > > > > > > > > > > > > > > > > > > > Brian > > > > > > > > > > > > > > > > > > > > > > fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- > > > > > > > > fs/xfs/scrub/common.c | 2 ++ > > > > > > > > fs/xfs/xfs_acl.c | 2 ++ > > > > > > > > fs/xfs/xfs_attr_item.c | 2 +- > > > > > > > > fs/xfs/xfs_ioctl.c | 2 ++ > > > > > > > > fs/xfs/xfs_ioctl32.c | 2 ++ > > > > > > > > fs/xfs/xfs_iops.c | 1 + > > > > > > > > fs/xfs/xfs_xattr.c | 1 + > > > > > > > > 8 files changed, 28 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > > > > > > > index 974c963..4ce3b0a 100644 > > > > > > > > --- a/fs/xfs/libxfs/xfs_attr.h > > > > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h > > > > > > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ > > > > > > > > char a_name[1]; /* attr name (NULL terminated) */ > > > > > > > > } attrlist_ent_t; > > > > > > > > +/* Attr state machine types */ > > > > > > > > +enum xfs_attr_state { > > > > > > > > + XFS_ATTR_STATE1 = 1, > > > > > > > > + XFS_ATTR_STATE2 = 2, > > > > > > > > + XFS_ATTR_STATE3 = 3, > > > > > > > > +}; > > > > > > > > + > > > > > > > > /* > > > > > > > > * List of attrs to commit later. > > > > > > > > */ > > > > > > > > @@ -88,7 +95,16 @@ struct xfs_attr_item { > > > > > > > > void *xattri_name; /* attr name */ > > > > > > > > uint32_t xattri_name_len; /* length of name */ > > > > > > > > uint32_t xattri_flags; /* attr flags */ > > > > > > > > - struct list_head xattri_list; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * Delayed attr parameters that need to remain instantiated > > > > > > > > + * across transaction rolls during the defer finish > > > > > > > > + */ > > > > > > > > + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ > > > > > > > > + enum xfs_attr_state xattri_state; /* state machine marker */ > > > > > > > > + struct xfs_da_args xattri_args; /* args context */ > > > > > > > > + > > > > > > > > + struct list_head xattri_list; > > > > > > > > /* > > > > > > > > * A byte array follows the header containing the file name and > > > > > > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c > > > > > > > > index 0c54ff5..270c32e 100644 > > > > > > > > --- a/fs/xfs/scrub/common.c > > > > > > > > +++ b/fs/xfs/scrub/common.c > > > > > > > > @@ -30,6 +30,8 @@ > > > > > > > > #include "xfs_rmap_btree.h" > > > > > > > > #include "xfs_log.h" > > > > > > > > #include "xfs_trans_priv.h" > > > > > > > > +#include "xfs_da_format.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_reflink.h" > > > > > > > > #include "scrub/xfs_scrub.h" > > > > > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c > > > > > > > > index 142de8d..9b1b93e 100644 > > > > > > > > --- a/fs/xfs/xfs_acl.c > > > > > > > > +++ b/fs/xfs/xfs_acl.c > > > > > > > > @@ -10,6 +10,8 @@ > > > > > > > > #include "xfs_mount.h" > > > > > > > > #include "xfs_inode.h" > > > > > > > > #include "xfs_acl.h" > > > > > > > > +#include "xfs_da_format.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_trace.h" > > > > > > > > #include <linux/slab.h> > > > > > > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > > > > > > > index 0ea19b4..36e6d1e 100644 > > > > > > > > --- a/fs/xfs/xfs_attr_item.c > > > > > > > > +++ b/fs/xfs/xfs_attr_item.c > > > > > > > > @@ -19,10 +19,10 @@ > > > > > > > > #include "xfs_rmap.h" > > > > > > > > #include "xfs_inode.h" > > > > > > > > #include "xfs_icache.h" > > > > > > > > -#include "xfs_attr.h" > > > > > > > > #include "xfs_shared.h" > > > > > > > > #include "xfs_da_format.h" > > > > > > > > #include "xfs_da_btree.h" > > > > > > > > +#include "xfs_attr.h" > > > > > > > > static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) > > > > > > > > { > > > > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > > > > > > index ab341d6..c8728ca 100644 > > > > > > > > --- a/fs/xfs/xfs_ioctl.c > > > > > > > > +++ b/fs/xfs/xfs_ioctl.c > > > > > > > > @@ -16,6 +16,8 @@ > > > > > > > > #include "xfs_rtalloc.h" > > > > > > > > #include "xfs_itable.h" > > > > > > > > #include "xfs_error.h" > > > > > > > > +#include "xfs_da_format.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_bmap.h" > > > > > > > > #include "xfs_bmap_util.h" > > > > > > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > > > > > > > > index 5001dca..23f6990 100644 > > > > > > > > --- a/fs/xfs/xfs_ioctl32.c > > > > > > > > +++ b/fs/xfs/xfs_ioctl32.c > > > > > > > > @@ -21,6 +21,8 @@ > > > > > > > > #include "xfs_fsops.h" > > > > > > > > #include "xfs_alloc.h" > > > > > > > > #include "xfs_rtalloc.h" > > > > > > > > +#include "xfs_da_format.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_ioctl.h" > > > > > > > > #include "xfs_ioctl32.h" > > > > > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > > > > > > > index e73c21a..561c467 100644 > > > > > > > > --- a/fs/xfs/xfs_iops.c > > > > > > > > +++ b/fs/xfs/xfs_iops.c > > > > > > > > @@ -17,6 +17,7 @@ > > > > > > > > #include "xfs_acl.h" > > > > > > > > #include "xfs_quota.h" > > > > > > > > #include "xfs_error.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_trans.h" > > > > > > > > #include "xfs_trace.h" > > > > > > > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > > > > > > > index 3013746..938e81d 100644 > > > > > > > > --- a/fs/xfs/xfs_xattr.c > > > > > > > > +++ b/fs/xfs/xfs_xattr.c > > > > > > > > @@ -11,6 +11,7 @@ > > > > > > > > #include "xfs_mount.h" > > > > > > > > #include "xfs_da_format.h" > > > > > > > > #include "xfs_inode.h" > > > > > > > > +#include "xfs_da_btree.h" > > > > > > > > #include "xfs_attr.h" > > > > > > > > #include "xfs_attr_leaf.h" > > > > > > > > #include "xfs_acl.h" > > > > > > > > -- > > > > > > > > 2.7.4 > > > > > > > >
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 974c963..4ce3b0a 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -77,6 +77,13 @@ typedef struct attrlist_ent { /* data from attr_list() */ char a_name[1]; /* attr name (NULL terminated) */ } attrlist_ent_t; +/* Attr state machine types */ +enum xfs_attr_state { + XFS_ATTR_STATE1 = 1, + XFS_ATTR_STATE2 = 2, + XFS_ATTR_STATE3 = 3, +}; + /* * List of attrs to commit later. */ @@ -88,7 +95,16 @@ struct xfs_attr_item { void *xattri_name; /* attr name */ uint32_t xattri_name_len; /* length of name */ uint32_t xattri_flags; /* attr flags */ - struct list_head xattri_list; + + /* + * Delayed attr parameters that need to remain instantiated + * across transaction rolls during the defer finish + */ + struct xfs_buf *xattri_leaf_bp; /* Leaf buf to release */ + enum xfs_attr_state xattri_state; /* state machine marker */ + struct xfs_da_args xattri_args; /* args context */ + + struct list_head xattri_list; /* * A byte array follows the header containing the file name and diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c index 0c54ff5..270c32e 100644 --- a/fs/xfs/scrub/common.c +++ b/fs/xfs/scrub/common.c @@ -30,6 +30,8 @@ #include "xfs_rmap_btree.h" #include "xfs_log.h" #include "xfs_trans_priv.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_reflink.h" #include "scrub/xfs_scrub.h" diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c index 142de8d..9b1b93e 100644 --- a/fs/xfs/xfs_acl.c +++ b/fs/xfs/xfs_acl.c @@ -10,6 +10,8 @@ #include "xfs_mount.h" #include "xfs_inode.h" #include "xfs_acl.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_trace.h" #include <linux/slab.h> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c index 0ea19b4..36e6d1e 100644 --- a/fs/xfs/xfs_attr_item.c +++ b/fs/xfs/xfs_attr_item.c @@ -19,10 +19,10 @@ #include "xfs_rmap.h" #include "xfs_inode.h" #include "xfs_icache.h" -#include "xfs_attr.h" #include "xfs_shared.h" #include "xfs_da_format.h" #include "xfs_da_btree.h" +#include "xfs_attr.h" static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip) { diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index ab341d6..c8728ca 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -16,6 +16,8 @@ #include "xfs_rtalloc.h" #include "xfs_itable.h" #include "xfs_error.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_bmap.h" #include "xfs_bmap_util.h" diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 5001dca..23f6990 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -21,6 +21,8 @@ #include "xfs_fsops.h" #include "xfs_alloc.h" #include "xfs_rtalloc.h" +#include "xfs_da_format.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_ioctl.h" #include "xfs_ioctl32.h" diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index e73c21a..561c467 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -17,6 +17,7 @@ #include "xfs_acl.h" #include "xfs_quota.h" #include "xfs_error.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_trans.h" #include "xfs_trace.h" diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 3013746..938e81d 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -11,6 +11,7 @@ #include "xfs_mount.h" #include "xfs_da_format.h" #include "xfs_inode.h" +#include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_attr_leaf.h" #include "xfs_acl.h"
This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer and a new state type. We will use these in the next patch when we modify xfs_set_attr_args to roll transactions by returning EAGAIN. Because the subroutines of this function modify the contents of these structures, we need to find a place to store them where they remain instantiated across multiple calls to xfs_set_attr_args. Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++- fs/xfs/scrub/common.c | 2 ++ fs/xfs/xfs_acl.c | 2 ++ fs/xfs/xfs_attr_item.c | 2 +- fs/xfs/xfs_ioctl.c | 2 ++ fs/xfs/xfs_ioctl32.c | 2 ++ fs/xfs/xfs_iops.c | 1 + fs/xfs/xfs_xattr.c | 1 + 8 files changed, 28 insertions(+), 2 deletions(-)