| Message ID | 1510942905-12897-18-git-send-email-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/17/2017 11:21 AM, Allison Henderson wrote: > This patch adds a new file ioctl to retrieve the parent > pointer of a given inode > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/xfs/libxfs/xfs_fs.h | 1 + > fs/xfs/xfs_attr.h | 2 ++ > fs/xfs/xfs_attr_list.c | 3 +++ > fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- > 5 files changed, 120 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 9d4d883..d2be842 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( > return xfs_attr_node_get(args); > } > > +/* > + * Get the parent pointer for a given inode > + * Caller will need to allocate a buffer pointed to by xpnir->p_name > + * and store the buffer size in xpnir->p_namelen. The parent > + * pointer will be stored in the given xfs_parent_name_irec > + * > + * Returns 0 on success and non zero on error > + */ > +int > +xfs_attr_get_parent_pointer(struct xfs_inode *ip, > + struct xfs_parent_name_irec *xpnir) > +{ > + struct attrlist *alist; > + struct attrlist_ent *aent; > + struct attrlist_cursor_kern cursor; > + struct xfs_parent_name_rec *xpnr; > + char *namebuf; > + int error = 0; > + unsigned int flags = ATTR_PARENT; > + > + /* Allocate a buffer to store the attribute names */ > + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); > + if (!namebuf) > + return -ENOMEM; > + > + /* Get all attribute names that have the ATTR_PARENT flag */ > + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); > + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); > + if (error) > + goto out_kfree; > + > + alist = (struct attrlist *)namebuf; > + > + /* There should never be more than one parent pointer */ > + ASSERT(alist->al_count == 1); > + > + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; > + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); > + > + /* > + * The value of the parent pointer attribute should be the file name > + * So we check the value length of the attribute entry against the name > + * length of the parent name record to make sure the caller gave enough > + * buffer space to store the file name (plus a null terminator) > + */ > + if (aent->a_valuelen >= xpnir->p_namelen) { > + error = -ERANGE; > + goto out_kfree; > + } > + > + xpnir->p_namelen = aent->a_valuelen + 1; > + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); > + error = xfs_attr_get(ip, (char *)xpnr, > + sizeof(struct xfs_parent_name_rec), > + (unsigned char *)(xpnir->p_name), > + (int *)&(xpnir->p_namelen), flags); > + if (error) > + goto out_kfree; > + > + xfs_init_parent_name_irec(xpnir, xpnr); > + > +out_kfree: > + kmem_free(namebuf); > + > + return error; > +} I was thinking of moving this function else where. It seems to generate a lot of compile issues when I apply it to xfsprogs because of the things it needs from xfs_attr.h. Generally are patches to code in fs/xfs/libxfs not supposed to be including things outside libxfs? Do I need to revise the series to avoid doing that? Thanks! Allison > + > /* Retrieve an extended attribute by name, and its value. */ > int > xfs_attr_get( > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index b8108f8..2f9ca2c 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -512,6 +512,7 @@ typedef struct xfs_swapext > #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) > #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) > /* XFS_IOC_GETFSMAP ------ hoisted 59 */ > +#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) > > /* > * ioctl commands that replace IRIX syssgi()'s > diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h > index 0829687..0ec3458 100644 > --- a/fs/xfs/xfs_attr.h > +++ b/fs/xfs/xfs_attr.h > @@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, > int flags); > int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, > size_t namelen, unsigned char *value, int valuelen, int flags); > +int xfs_attr_get_parent_pointer(struct xfs_inode *ip, > + struct xfs_parent_name_irec *xpnir); > int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); > int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, > size_t namelen, int flags); > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > index 7740c8a..78fc477 100644 > --- a/fs/xfs/xfs_attr_list.c > +++ b/fs/xfs/xfs_attr_list.c > @@ -534,6 +534,9 @@ xfs_attr_put_listent( > if (((context->flags & ATTR_ROOT) == 0) != > ((flags & XFS_ATTR_ROOT) == 0)) > return; > + if (((context->flags & ATTR_PARENT) == 0) != > + ((flags & XFS_ATTR_PARENT) == 0)) > + return; > > arraytop = sizeof(*alist) + > context->count * sizeof(alist->al_offset[0]); > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 4664314..5492607 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -44,6 +44,7 @@ > #include "xfs_btree.h" > #include <linux/fsmap.h> > #include "xfs_fsmap.h" > +#include "xfs_attr.h" > > #include <linux/capability.h> > #include <linux/cred.h> > @@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( > return 0; > } > > +/* > + * IOCTL routine to get the parent pointer of an inode and return it to user > + * space. Caller must pass an struct xfs_parent_name_irec with a name buffer > + * large enough to hold the file name. Returns 0 on success or non-zero on > + * failure > + */ > +STATIC int > +xfs_ioc_get_parent_pointer( > + struct file *filp, > + void __user *arg) > +{ > + struct inode *inode = file_inode(filp); > + struct xfs_inode *ip = XFS_I(inode); > + struct xfs_parent_name_irec xpnir; > + char *uname; > + char *kname; > + int error = 0; > + > + copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); > + uname = (char *)xpnir.p_name; > + > + /* > + * Use kernel space memory to get the parent pointer name. > + * We'll copy it to the user space name back when we're done > + */ > + kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); > + if (!kname) > + return -ENOMEM; > + > + xpnir.p_name = kname; > + error = xfs_attr_get_parent_pointer(ip, &xpnir); > + > + if (error) > + goto out; > + > + copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); > + xpnir.p_name = uname; > + copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); > + > +out: > + kmem_free(kname); > + return error; > +} > + > int > xfs_ioc_swapext( > xfs_swapext_t *sxp) > @@ -1866,7 +1911,8 @@ xfs_file_ioctl( > return xfs_ioc_getxflags(ip, arg); > case XFS_IOC_SETXFLAGS: > return xfs_ioc_setxflags(ip, filp, arg); > - > + case XFS_IOC_GETPPOINTER: > + return xfs_ioc_get_parent_pointer(filp, arg); > case XFS_IOC_FSSETDM: { > struct fsdmidata dmi; > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 22, 2017 at 12:54:45PM -0700, Allison Henderson wrote: > On 11/17/2017 11:21 AM, Allison Henderson wrote: > > >This patch adds a new file ioctl to retrieve the parent > >pointer of a given inode > > > >Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > >--- > > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ > > fs/xfs/libxfs/xfs_fs.h | 1 + > > fs/xfs/xfs_attr.h | 2 ++ > > fs/xfs/xfs_attr_list.c | 3 +++ > > fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- > > 5 files changed, 120 insertions(+), 1 deletion(-) > > > >diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > >index 9d4d883..d2be842 100644 > >--- a/fs/xfs/libxfs/xfs_attr.c > >+++ b/fs/xfs/libxfs/xfs_attr.c > >@@ -134,6 +134,73 @@ xfs_attr_get_ilocked( > > return xfs_attr_node_get(args); > > } > >+/* > >+ * Get the parent pointer for a given inode > >+ * Caller will need to allocate a buffer pointed to by xpnir->p_name > >+ * and store the buffer size in xpnir->p_namelen. The parent > >+ * pointer will be stored in the given xfs_parent_name_irec > >+ * > >+ * Returns 0 on success and non zero on error > >+ */ > >+int > >+xfs_attr_get_parent_pointer(struct xfs_inode *ip, > >+ struct xfs_parent_name_irec *xpnir) > >+{ > >+ struct attrlist *alist; > >+ struct attrlist_ent *aent; > >+ struct attrlist_cursor_kern cursor; > >+ struct xfs_parent_name_rec *xpnr; > >+ char *namebuf; > >+ int error = 0; > >+ unsigned int flags = ATTR_PARENT; > >+ > >+ /* Allocate a buffer to store the attribute names */ > >+ namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); > >+ if (!namebuf) > >+ return -ENOMEM; > >+ > >+ /* Get all attribute names that have the ATTR_PARENT flag */ > >+ memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); > >+ error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); > >+ if (error) > >+ goto out_kfree; > >+ > >+ alist = (struct attrlist *)namebuf; > >+ > >+ /* There should never be more than one parent pointer */ > >+ ASSERT(alist->al_count == 1); > >+ > >+ aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; > >+ xpnr = (struct xfs_parent_name_rec *)(aent->a_name); > >+ > >+ /* > >+ * The value of the parent pointer attribute should be the file name > >+ * So we check the value length of the attribute entry against the name > >+ * length of the parent name record to make sure the caller gave enough > >+ * buffer space to store the file name (plus a null terminator) > >+ */ > >+ if (aent->a_valuelen >= xpnir->p_namelen) { > >+ error = -ERANGE; > >+ goto out_kfree; > >+ } > >+ > >+ xpnir->p_namelen = aent->a_valuelen + 1; > >+ memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); > >+ error = xfs_attr_get(ip, (char *)xpnr, > >+ sizeof(struct xfs_parent_name_rec), > >+ (unsigned char *)(xpnir->p_name), > >+ (int *)&(xpnir->p_namelen), flags); > >+ if (error) > >+ goto out_kfree; > >+ > >+ xfs_init_parent_name_irec(xpnir, xpnr); > >+ > >+out_kfree: > >+ kmem_free(namebuf); > >+ > >+ return error; > >+} > I was thinking of moving this function else where. It seems to > generate a lot of compile issues when I apply it to xfsprogs because > of the things it needs from xfs_attr.h. Generally are patches to > code in fs/xfs/libxfs not supposed to be including things outside > libxfs? Do I need to revise the series to avoid doing that? Thanks! In general, yes. More complex than that (e.g. userspace and kernel have separate definitions of some structures like xfs_mount, xfs_buf, etc), but we try to keep the libxfs code as encapsulated as possible. In terms of getting attrs to userspace, the equivalent attribute listing code is in fs/xfs/xfs_attr_list.c, and that avoids all these problems. I'd just move the xfs_attr_get_parent_pointer() function there as ithis code should not be needed in userspace and it would avoid all the userspace libxfs compile issues... Cheers, Dave.
On Wed, Nov 22, 2017 at 12:54:45PM -0700, Allison Henderson wrote: > On 11/17/2017 11:21 AM, Allison Henderson wrote: > > >This patch adds a new file ioctl to retrieve the parent > >pointer of a given inode > > > >Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > >--- > > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ > > fs/xfs/libxfs/xfs_fs.h | 1 + > > fs/xfs/xfs_attr.h | 2 ++ > > fs/xfs/xfs_attr_list.c | 3 +++ > > fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- > > 5 files changed, 120 insertions(+), 1 deletion(-) > > > >diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > >index 9d4d883..d2be842 100644 > >--- a/fs/xfs/libxfs/xfs_attr.c > >+++ b/fs/xfs/libxfs/xfs_attr.c > >@@ -134,6 +134,73 @@ xfs_attr_get_ilocked( > > return xfs_attr_node_get(args); > > } > >+/* > >+ * Get the parent pointer for a given inode > >+ * Caller will need to allocate a buffer pointed to by xpnir->p_name > >+ * and store the buffer size in xpnir->p_namelen. The parent > >+ * pointer will be stored in the given xfs_parent_name_irec > >+ * > >+ * Returns 0 on success and non zero on error > >+ */ > >+int > >+xfs_attr_get_parent_pointer(struct xfs_inode *ip, > >+ struct xfs_parent_name_irec *xpnir) > >+{ > >+ struct attrlist *alist; > >+ struct attrlist_ent *aent; > >+ struct attrlist_cursor_kern cursor; > >+ struct xfs_parent_name_rec *xpnr; > >+ char *namebuf; > >+ int error = 0; > >+ unsigned int flags = ATTR_PARENT; > >+ > >+ /* Allocate a buffer to store the attribute names */ > >+ namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); > >+ if (!namebuf) > >+ return -ENOMEM; > >+ > >+ /* Get all attribute names that have the ATTR_PARENT flag */ > >+ memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); > >+ error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); > >+ if (error) > >+ goto out_kfree; > >+ > >+ alist = (struct attrlist *)namebuf; > >+ > >+ /* There should never be more than one parent pointer */ > >+ ASSERT(alist->al_count == 1); /me wonders, does this handle hardlinked files correctly? > >+ aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; > >+ xpnr = (struct xfs_parent_name_rec *)(aent->a_name); > >+ > >+ /* > >+ * The value of the parent pointer attribute should be the file name > >+ * So we check the value length of the attribute entry against the name > >+ * length of the parent name record to make sure the caller gave enough > >+ * buffer space to store the file name (plus a null terminator) > >+ */ > >+ if (aent->a_valuelen >= xpnir->p_namelen) { > >+ error = -ERANGE; > >+ goto out_kfree; > >+ } > >+ > >+ xpnir->p_namelen = aent->a_valuelen + 1; > >+ memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); > >+ error = xfs_attr_get(ip, (char *)xpnr, > >+ sizeof(struct xfs_parent_name_rec), > >+ (unsigned char *)(xpnir->p_name), > >+ (int *)&(xpnir->p_namelen), flags); > >+ if (error) > >+ goto out_kfree; > >+ > >+ xfs_init_parent_name_irec(xpnir, xpnr); > >+ > >+out_kfree: > >+ kmem_free(namebuf); > >+ > >+ return error; > >+} > I was thinking of moving this function else where. It seems to generate a > lot of compile issues when I apply it to xfsprogs because of the things it > needs from xfs_attr.h. note: i forget what this function does exactly. :/ Heh. Yeah, you might need to split the parent pointer code into fs/xfs/libxfs/xfs_parent_ptr.c that handles all the internal work and a fs/xfs/xfs_parent.c that glues the kernel to libxfs, similar to how the directory code is split up. IOWs, fs/xfs/libxfs/xfs_parent_ptr.c has routines to set/clear xfs_parent_irec structures by modifying xattr data as appropriate; and iterate all the theoretical xfs_parent_irecs based on what's in the xattr data. fs/xfs/xfs_parent.c then has all the glue code to connect the iterator interface to ioctls, etc. > Generally are patches to code in fs/xfs/libxfs not supposed to be > including things outside libxfs? You'd think so, but yesno. :P In general we'd prefer libxfs to be as self-contained as possible so that xfsprogs/kernel have exactly the same libxfs code. OTOH the practical reality of both libxfs's is that they sometimes need things that are defined outside of libxfs. That said, I'll now undercut my own point by noting that libxfs is really just common code shared between the two codebases that want it. > Do I need to revise the series to avoid doing that? Thanks! So it's not a hard and fast rule, just more of a "minimize the libxfs dependencies on the outer world" thing. (Those outer world things force Eric to fix them up in whatever odd way xfsprogs needs because it's userspace.) --D > > Allison > >+ > > /* Retrieve an extended attribute by name, and its value. */ > > int > > xfs_attr_get( > >diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > >index b8108f8..2f9ca2c 100644 > >--- a/fs/xfs/libxfs/xfs_fs.h > >+++ b/fs/xfs/libxfs/xfs_fs.h > >@@ -512,6 +512,7 @@ typedef struct xfs_swapext > > #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) > > #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) > > /* XFS_IOC_GETFSMAP ------ hoisted 59 */ > >+#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) > > /* > > * ioctl commands that replace IRIX syssgi()'s > >diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h > >index 0829687..0ec3458 100644 > >--- a/fs/xfs/xfs_attr.h > >+++ b/fs/xfs/xfs_attr.h > >@@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, > > int flags); > > int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, > > size_t namelen, unsigned char *value, int valuelen, int flags); > >+int xfs_attr_get_parent_pointer(struct xfs_inode *ip, > >+ struct xfs_parent_name_irec *xpnir); > > int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); > > int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, > > size_t namelen, int flags); > >diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > >index 7740c8a..78fc477 100644 > >--- a/fs/xfs/xfs_attr_list.c > >+++ b/fs/xfs/xfs_attr_list.c > >@@ -534,6 +534,9 @@ xfs_attr_put_listent( > > if (((context->flags & ATTR_ROOT) == 0) != > > ((flags & XFS_ATTR_ROOT) == 0)) > > return; > >+ if (((context->flags & ATTR_PARENT) == 0) != > >+ ((flags & XFS_ATTR_PARENT) == 0)) > >+ return; > > arraytop = sizeof(*alist) + > > context->count * sizeof(alist->al_offset[0]); > >diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > >index 4664314..5492607 100644 > >--- a/fs/xfs/xfs_ioctl.c > >+++ b/fs/xfs/xfs_ioctl.c > >@@ -44,6 +44,7 @@ > > #include "xfs_btree.h" > > #include <linux/fsmap.h> > > #include "xfs_fsmap.h" > >+#include "xfs_attr.h" > > #include <linux/capability.h> > > #include <linux/cred.h> > >@@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( > > return 0; > > } > >+/* > >+ * IOCTL routine to get the parent pointer of an inode and return it to user > >+ * space. Caller must pass an struct xfs_parent_name_irec with a name buffer > >+ * large enough to hold the file name. Returns 0 on success or non-zero on > >+ * failure > >+ */ > >+STATIC int > >+xfs_ioc_get_parent_pointer( > >+ struct file *filp, > >+ void __user *arg) > >+{ > >+ struct inode *inode = file_inode(filp); > >+ struct xfs_inode *ip = XFS_I(inode); > >+ struct xfs_parent_name_irec xpnir; > >+ char *uname; > >+ char *kname; > >+ int error = 0; > >+ > >+ copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); > >+ uname = (char *)xpnir.p_name; > >+ > >+ /* > >+ * Use kernel space memory to get the parent pointer name. > >+ * We'll copy it to the user space name back when we're done > >+ */ > >+ kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); > >+ if (!kname) > >+ return -ENOMEM; > >+ > >+ xpnir.p_name = kname; > >+ error = xfs_attr_get_parent_pointer(ip, &xpnir); > >+ > >+ if (error) > >+ goto out; > >+ > >+ copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); > >+ xpnir.p_name = uname; > >+ copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); > >+ > >+out: > >+ kmem_free(kname); > >+ return error; > >+} > >+ > > int > > xfs_ioc_swapext( > > xfs_swapext_t *sxp) > >@@ -1866,7 +1911,8 @@ xfs_file_ioctl( > > return xfs_ioc_getxflags(ip, arg); > > case XFS_IOC_SETXFLAGS: > > return xfs_ioc_setxflags(ip, filp, arg); > >- > >+ case XFS_IOC_GETPPOINTER: > >+ return xfs_ioc_get_parent_pointer(filp, arg); > > case XFS_IOC_FSSETDM: { > > struct fsdmidata dmi; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/22/2017 02:07 PM, Dave Chinner wrote: > On Wed, Nov 22, 2017 at 12:54:45PM -0700, Allison Henderson wrote: >> On 11/17/2017 11:21 AM, Allison Henderson wrote: >> >>> This patch adds a new file ioctl to retrieve the parent >>> pointer of a given inode >>> >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>> --- >>> fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> fs/xfs/libxfs/xfs_fs.h | 1 + >>> fs/xfs/xfs_attr.h | 2 ++ >>> fs/xfs/xfs_attr_list.c | 3 +++ >>> fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- >>> 5 files changed, 120 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>> index 9d4d883..d2be842 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.c >>> +++ b/fs/xfs/libxfs/xfs_attr.c >>> @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( >>> return xfs_attr_node_get(args); >>> } >>> +/* >>> + * Get the parent pointer for a given inode >>> + * Caller will need to allocate a buffer pointed to by xpnir->p_name >>> + * and store the buffer size in xpnir->p_namelen. The parent >>> + * pointer will be stored in the given xfs_parent_name_irec >>> + * >>> + * Returns 0 on success and non zero on error >>> + */ >>> +int >>> +xfs_attr_get_parent_pointer(struct xfs_inode *ip, >>> + struct xfs_parent_name_irec *xpnir) >>> +{ >>> + struct attrlist *alist; >>> + struct attrlist_ent *aent; >>> + struct attrlist_cursor_kern cursor; >>> + struct xfs_parent_name_rec *xpnr; >>> + char *namebuf; >>> + int error = 0; >>> + unsigned int flags = ATTR_PARENT; >>> + >>> + /* Allocate a buffer to store the attribute names */ >>> + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); >>> + if (!namebuf) >>> + return -ENOMEM; >>> + >>> + /* Get all attribute names that have the ATTR_PARENT flag */ >>> + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); >>> + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); >>> + if (error) >>> + goto out_kfree; >>> + >>> + alist = (struct attrlist *)namebuf; >>> + >>> + /* There should never be more than one parent pointer */ >>> + ASSERT(alist->al_count == 1); >>> + >>> + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; >>> + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); >>> + >>> + /* >>> + * The value of the parent pointer attribute should be the file name >>> + * So we check the value length of the attribute entry against the name >>> + * length of the parent name record to make sure the caller gave enough >>> + * buffer space to store the file name (plus a null terminator) >>> + */ >>> + if (aent->a_valuelen >= xpnir->p_namelen) { >>> + error = -ERANGE; >>> + goto out_kfree; >>> + } >>> + >>> + xpnir->p_namelen = aent->a_valuelen + 1; >>> + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); >>> + error = xfs_attr_get(ip, (char *)xpnr, >>> + sizeof(struct xfs_parent_name_rec), >>> + (unsigned char *)(xpnir->p_name), >>> + (int *)&(xpnir->p_namelen), flags); >>> + if (error) >>> + goto out_kfree; >>> + >>> + xfs_init_parent_name_irec(xpnir, xpnr); >>> + >>> +out_kfree: >>> + kmem_free(namebuf); >>> + >>> + return error; >>> +} >> I was thinking of moving this function else where. It seems to >> generate a lot of compile issues when I apply it to xfsprogs because >> of the things it needs from xfs_attr.h. Generally are patches to >> code in fs/xfs/libxfs not supposed to be including things outside >> libxfs? Do I need to revise the series to avoid doing that? Thanks! > In general, yes. More complex than that (e.g. userspace and kernel > have separate definitions of some structures like xfs_mount, > xfs_buf, etc), but we try to keep the libxfs code as encapsulated as > possible. > > In terms of getting attrs to userspace, the equivalent attribute > listing code is in fs/xfs/xfs_attr_list.c, and that avoids all these > problems. I'd just move the xfs_attr_get_parent_pointer() function > there as ithis code should not be needed in userspace and it would > avoid all the userspace libxfs compile issues... > > Cheers, > > Dave. Alrighty, that seems like a good place for it then. Thank you! Allison -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/22/2017 02:13 PM, Darrick J. Wong wrote: > On Wed, Nov 22, 2017 at 12:54:45PM -0700, Allison Henderson wrote: >> On 11/17/2017 11:21 AM, Allison Henderson wrote: >> >>> This patch adds a new file ioctl to retrieve the parent >>> pointer of a given inode >>> >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >>> --- >>> fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> fs/xfs/libxfs/xfs_fs.h | 1 + >>> fs/xfs/xfs_attr.h | 2 ++ >>> fs/xfs/xfs_attr_list.c | 3 +++ >>> fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- >>> 5 files changed, 120 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>> index 9d4d883..d2be842 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.c >>> +++ b/fs/xfs/libxfs/xfs_attr.c >>> @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( >>> return xfs_attr_node_get(args); >>> } >>> +/* >>> + * Get the parent pointer for a given inode >>> + * Caller will need to allocate a buffer pointed to by xpnir->p_name >>> + * and store the buffer size in xpnir->p_namelen. The parent >>> + * pointer will be stored in the given xfs_parent_name_irec >>> + * >>> + * Returns 0 on success and non zero on error >>> + */ >>> +int >>> +xfs_attr_get_parent_pointer(struct xfs_inode *ip, >>> + struct xfs_parent_name_irec *xpnir) >>> +{ >>> + struct attrlist *alist; >>> + struct attrlist_ent *aent; >>> + struct attrlist_cursor_kern cursor; >>> + struct xfs_parent_name_rec *xpnr; >>> + char *namebuf; >>> + int error = 0; >>> + unsigned int flags = ATTR_PARENT; >>> + >>> + /* Allocate a buffer to store the attribute names */ >>> + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); >>> + if (!namebuf) >>> + return -ENOMEM; >>> + >>> + /* Get all attribute names that have the ATTR_PARENT flag */ >>> + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); >>> + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); >>> + if (error) >>> + goto out_kfree; >>> + >>> + alist = (struct attrlist *)namebuf; >>> + >>> + /* There should never be more than one parent pointer */ >>> + ASSERT(alist->al_count == 1); > /me wonders, does this handle hardlinked files correctly? Good question, I will try it out and maybe revise this area a bit. Maybe we need to be returning more than one parent pointer > >>> + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; >>> + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); >>> + >>> + /* >>> + * The value of the parent pointer attribute should be the file name >>> + * So we check the value length of the attribute entry against the name >>> + * length of the parent name record to make sure the caller gave enough >>> + * buffer space to store the file name (plus a null terminator) >>> + */ >>> + if (aent->a_valuelen >= xpnir->p_namelen) { >>> + error = -ERANGE; >>> + goto out_kfree; >>> + } >>> + >>> + xpnir->p_namelen = aent->a_valuelen + 1; >>> + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); >>> + error = xfs_attr_get(ip, (char *)xpnr, >>> + sizeof(struct xfs_parent_name_rec), >>> + (unsigned char *)(xpnir->p_name), >>> + (int *)&(xpnir->p_namelen), flags); >>> + if (error) >>> + goto out_kfree; >>> + >>> + xfs_init_parent_name_irec(xpnir, xpnr); >>> + >>> +out_kfree: >>> + kmem_free(namebuf); >>> + >>> + return error; >>> +} >> I was thinking of moving this function else where. It seems to generate a >> lot of compile issues when I apply it to xfsprogs because of the things it >> needs from xfs_attr.h. > note: i forget what this function does exactly. :/ It just picks out the name tuple from the parent pointer attribute and and uses it to look up the attribute value (the file name). All that gets stored in the xfs_parent_name_irec that the callers passes in. > > Heh. Yeah, you might need to split the parent pointer code into > fs/xfs/libxfs/xfs_parent_ptr.c that handles all the internal work and a > fs/xfs/xfs_parent.c that glues the kernel to libxfs, similar to how the > directory code is split up. > > IOWs, fs/xfs/libxfs/xfs_parent_ptr.c has routines to set/clear > xfs_parent_irec structures by modifying xattr data as appropriate; and > iterate all the theoretical xfs_parent_irecs based on what's in the > xattr data. > > fs/xfs/xfs_parent.c then has all the glue code to connect the iterator > interface to ioctls, etc. Yeah, I may need to revisit some of that as I go about putting together what I need for new xfstests then >> Generally are patches to code in fs/xfs/libxfs not supposed to be >> including things outside libxfs? > You'd think so, but yesno. :P > > In general we'd prefer libxfs to be as self-contained as possible so > that xfsprogs/kernel have exactly the same libxfs code. OTOH the > practical reality of both libxfs's is that they sometimes need things > that are defined outside of libxfs. > > That said, I'll now undercut my own point by noting that libxfs is > really just common code shared between the two codebases that want it. > >> Do I need to revise the series to avoid doing that? Thanks! > So it's not a hard and fast rule, just more of a "minimize the libxfs > dependencies on the outer world" thing. > > (Those outer world things force Eric to fix them up in whatever > odd way xfsprogs needs because it's userspace.) > > --D Alrighty, I'll try to keep everything as much as possible. Thanks all! Allison >> Allison >>> + >>> /* Retrieve an extended attribute by name, and its value. */ >>> int >>> xfs_attr_get( >>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h >>> index b8108f8..2f9ca2c 100644 >>> --- a/fs/xfs/libxfs/xfs_fs.h >>> +++ b/fs/xfs/libxfs/xfs_fs.h >>> @@ -512,6 +512,7 @@ typedef struct xfs_swapext >>> #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) >>> #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) >>> /* XFS_IOC_GETFSMAP ------ hoisted 59 */ >>> +#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) >>> /* >>> * ioctl commands that replace IRIX syssgi()'s >>> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h >>> index 0829687..0ec3458 100644 >>> --- a/fs/xfs/xfs_attr.h >>> +++ b/fs/xfs/xfs_attr.h >>> @@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, >>> int flags); >>> int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, >>> size_t namelen, unsigned char *value, int valuelen, int flags); >>> +int xfs_attr_get_parent_pointer(struct xfs_inode *ip, >>> + struct xfs_parent_name_irec *xpnir); >>> int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); >>> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, >>> size_t namelen, int flags); >>> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c >>> index 7740c8a..78fc477 100644 >>> --- a/fs/xfs/xfs_attr_list.c >>> +++ b/fs/xfs/xfs_attr_list.c >>> @@ -534,6 +534,9 @@ xfs_attr_put_listent( >>> if (((context->flags & ATTR_ROOT) == 0) != >>> ((flags & XFS_ATTR_ROOT) == 0)) >>> return; >>> + if (((context->flags & ATTR_PARENT) == 0) != >>> + ((flags & XFS_ATTR_PARENT) == 0)) >>> + return; >>> arraytop = sizeof(*alist) + >>> context->count * sizeof(alist->al_offset[0]); >>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >>> index 4664314..5492607 100644 >>> --- a/fs/xfs/xfs_ioctl.c >>> +++ b/fs/xfs/xfs_ioctl.c >>> @@ -44,6 +44,7 @@ >>> #include "xfs_btree.h" >>> #include <linux/fsmap.h> >>> #include "xfs_fsmap.h" >>> +#include "xfs_attr.h" >>> #include <linux/capability.h> >>> #include <linux/cred.h> >>> @@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( >>> return 0; >>> } >>> +/* >>> + * IOCTL routine to get the parent pointer of an inode and return it to user >>> + * space. Caller must pass an struct xfs_parent_name_irec with a name buffer >>> + * large enough to hold the file name. Returns 0 on success or non-zero on >>> + * failure >>> + */ >>> +STATIC int >>> +xfs_ioc_get_parent_pointer( >>> + struct file *filp, >>> + void __user *arg) >>> +{ >>> + struct inode *inode = file_inode(filp); >>> + struct xfs_inode *ip = XFS_I(inode); >>> + struct xfs_parent_name_irec xpnir; >>> + char *uname; >>> + char *kname; >>> + int error = 0; >>> + >>> + copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); >>> + uname = (char *)xpnir.p_name; >>> + >>> + /* >>> + * Use kernel space memory to get the parent pointer name. >>> + * We'll copy it to the user space name back when we're done >>> + */ >>> + kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); >>> + if (!kname) >>> + return -ENOMEM; >>> + >>> + xpnir.p_name = kname; >>> + error = xfs_attr_get_parent_pointer(ip, &xpnir); >>> + >>> + if (error) >>> + goto out; >>> + >>> + copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); >>> + xpnir.p_name = uname; >>> + copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); >>> + >>> +out: >>> + kmem_free(kname); >>> + return error; >>> +} >>> + >>> int >>> xfs_ioc_swapext( >>> xfs_swapext_t *sxp) >>> @@ -1866,7 +1911,8 @@ xfs_file_ioctl( >>> return xfs_ioc_getxflags(ip, arg); >>> case XFS_IOC_SETXFLAGS: >>> return xfs_ioc_setxflags(ip, filp, arg); >>> - >>> + case XFS_IOC_GETPPOINTER: >>> + return xfs_ioc_get_parent_pointer(filp, arg); >>> case XFS_IOC_FSSETDM: { >>> struct fsdmidata dmi; >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIDAw&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=G7N7y48j4ogYpf2u666QU1bTWEMPTiSyJA2qT5hBpkQ&s=OJkTAOSkzCECkmU6FhK-vc77mLJpqb65wuKPpdpCGSA&e= > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIDAw&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=G7N7y48j4ogYpf2u666QU1bTWEMPTiSyJA2qT5hBpkQ&s=OJkTAOSkzCECkmU6FhK-vc77mLJpqb65wuKPpdpCGSA&e= -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: > This patch adds a new file ioctl to retrieve the parent > pointer of a given inode > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ > fs/xfs/libxfs/xfs_fs.h | 1 + > fs/xfs/xfs_attr.h | 2 ++ > fs/xfs/xfs_attr_list.c | 3 +++ > fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- > 5 files changed, 120 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 9d4d883..d2be842 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( > return xfs_attr_node_get(args); > } > > +/* > + * Get the parent pointer for a given inode > + * Caller will need to allocate a buffer pointed to by xpnir->p_name > + * and store the buffer size in xpnir->p_namelen. The parent > + * pointer will be stored in the given xfs_parent_name_irec > + * > + * Returns 0 on success and non zero on error > + */ > +int > +xfs_attr_get_parent_pointer(struct xfs_inode *ip, > + struct xfs_parent_name_irec *xpnir) Please fix the parameter list here. > +{ > + struct attrlist *alist; > + struct attrlist_ent *aent; > + struct attrlist_cursor_kern cursor; > + struct xfs_parent_name_rec *xpnr; > + char *namebuf; > + int error = 0; > + unsigned int flags = ATTR_PARENT; > + > + /* Allocate a buffer to store the attribute names */ > + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); > + if (!namebuf) > + return -ENOMEM; > + > + /* Get all attribute names that have the ATTR_PARENT flag */ > + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); > + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); > + if (error) > + goto out_kfree; > + > + alist = (struct attrlist *)namebuf; > + > + /* There should never be more than one parent pointer */ > + ASSERT(alist->al_count == 1); As mentioned earlier, this is not true. Files can have multiple parents. > + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; > + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); > + > + /* > + * The value of the parent pointer attribute should be the file name > + * So we check the value length of the attribute entry against the name > + * length of the parent name record to make sure the caller gave enough > + * buffer space to store the file name (plus a null terminator) > + */ > + if (aent->a_valuelen >= xpnir->p_namelen) { > + error = -ERANGE; > + goto out_kfree; > + } > + > + xpnir->p_namelen = aent->a_valuelen + 1; > + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); > + error = xfs_attr_get(ip, (char *)xpnr, > + sizeof(struct xfs_parent_name_rec), > + (unsigned char *)(xpnir->p_name), > + (int *)&(xpnir->p_namelen), flags); > + if (error) > + goto out_kfree; > + > + xfs_init_parent_name_irec(xpnir, xpnr); > + > +out_kfree: > + kmem_free(namebuf); > + > + return error; > +} > + > /* Retrieve an extended attribute by name, and its value. */ > int > xfs_attr_get( > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index b8108f8..2f9ca2c 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -512,6 +512,7 @@ typedef struct xfs_swapext > #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) > #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) > /* XFS_IOC_GETFSMAP ------ hoisted 59 */ > +#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) I don't think it's a good idea to expose internal data structures directly to userspace, because that inhibits our ability to change the in-core data structure. Furthermore, hardlinked files can have multiple parent pointers, so it's not going to suffice to return a single parent pointer entry. Given that there can be potentially 2^32 parents, we're going to need a data structure for the ioctl to store (in an opaque manner) the attribute iteration cursor and have space to pass back some number of parent pointers. (Yes, it's time to start talking about actual use cases...) At a bare minimum, this is what I pictured for the "return parents of the open file" ioctl: #define XFS_PPTR_MAXNAMELEN 255 struct xfs_pptr { u64 pp_ino; u32 pp_gen; u8 pp_namelen; u8 pp_name[XFS_PPTR_MAXNAMELEN]; }; /* return parents of the handle, instead of the open fd */ #define XFS_PPTR_FLAG_HANDLE (1u << 0) struct xfs_pptr_info { struct xfs_fsop_handlereq pi_handle; struct xfs_attrlist_cursor pi_cursor; u32 pi_flags; u32 pi_reserved; u32 pi_ptrs_size; u32 pi_ptrs_used; u64 pi_reserved2[6]; struct xfs_pptr pi_ptrs[0]; }; #define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ ((ptrs) * sizeof(struct xfs_pptr))); static inline struct xfs_pptr_info * xfs_pptr_alloc( size_t nr_ptrs) { struct xfs_pptr_info *ppi; ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); if (!ppi) return NULL; memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); ppi->pi_ptrs_size = nr_ptrs; return ppi; } With the following example userspace program (that does no checking whatsoever): int main(int argc, char *argv[]) { struct xfs_pptr_info *ppi; struct xfs_pptr *pp; int fd; fd = open(argv[1], O_RDONLY); ppi = xfs_pptr_alloc(32); while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { for (i = 0; i < ppi->pi_ptrs_used; i++) { printf("%llu:%u -> %s\n", ppi->pi_ptrs[i].pp_ino, ppi->pi_ptrs[i].pp_gen, ppi->pi_ptrs[i].pp_name); } } } Notice here how we the userspace structure contains an opaque attribute list cursor, so we can keep coming back for more parent pointers until we run out of xattrs (and pi_ptrs_used == 0). The kernel will copy its internal cursor out to the userspace buffer as an opaque cookie prior to returning. From this simple implementation it shouldn't be difficult to finish the parents_by_handle/parentpaths_by_handle functions in libhandle, though given that they've never been implemented in Linux and we no longer care about Irix, you've some flexibility to change those library functions if that is convenient for setting up xfstests. Speaking of xfstests... what are the initial test cases? I figured at least the following: 0) mkfs with protofile, make sure the parent records get created 1) create file, check parent records 2) hardlink file, check both parent records 3) delete one link of a hardlinked file, check parent records 4) hardlink a file a few thousand times, check that the iteration scheme laid out above actually works 5) rename a file within a directory, check the parent records 6) rename a file across directories, check the parent records 7) some sort of testing where we run out of space while updating pptrs 8) add some error injection knobs to make sure that pptr replay actually works correctly Can you think of other test cases? For xfs_scrub, we want to be able to query the parents of any (damaged) inode we find in the filesystem. If the inode is so damaged we can't open it (or it's a special file) then scrub has to construct a file handle and pass that in via pi_handle. I /also/ wonder if there's any interest in having a fallback for non-pptr filesystems that walks the dentry->d_parent links (like d_paths() does) back to the root. Such a fallback will only work on an opened dir or a file opened by path (i.e. not a handle), however, which limits its appeal. --D > /* > * ioctl commands that replace IRIX syssgi()'s > diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h > index 0829687..0ec3458 100644 > --- a/fs/xfs/xfs_attr.h > +++ b/fs/xfs/xfs_attr.h > @@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, > int flags); > int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, > size_t namelen, unsigned char *value, int valuelen, int flags); > +int xfs_attr_get_parent_pointer(struct xfs_inode *ip, > + struct xfs_parent_name_irec *xpnir); > int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); > int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, > size_t namelen, int flags); > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > index 7740c8a..78fc477 100644 > --- a/fs/xfs/xfs_attr_list.c > +++ b/fs/xfs/xfs_attr_list.c > @@ -534,6 +534,9 @@ xfs_attr_put_listent( > if (((context->flags & ATTR_ROOT) == 0) != > ((flags & XFS_ATTR_ROOT) == 0)) > return; > + if (((context->flags & ATTR_PARENT) == 0) != > + ((flags & XFS_ATTR_PARENT) == 0)) > + return; > > arraytop = sizeof(*alist) + > context->count * sizeof(alist->al_offset[0]); > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 4664314..5492607 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -44,6 +44,7 @@ > #include "xfs_btree.h" > #include <linux/fsmap.h> > #include "xfs_fsmap.h" > +#include "xfs_attr.h" > > #include <linux/capability.h> > #include <linux/cred.h> > @@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( > return 0; > } > > +/* > + * IOCTL routine to get the parent pointer of an inode and return it to user > + * space. Caller must pass an struct xfs_parent_name_irec with a name buffer > + * large enough to hold the file name. Returns 0 on success or non-zero on > + * failure > + */ > +STATIC int > +xfs_ioc_get_parent_pointer( > + struct file *filp, > + void __user *arg) > +{ > + struct inode *inode = file_inode(filp); > + struct xfs_inode *ip = XFS_I(inode); > + struct xfs_parent_name_irec xpnir; > + char *uname; > + char *kname; > + int error = 0; > + > + copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); > + uname = (char *)xpnir.p_name; > + > + /* > + * Use kernel space memory to get the parent pointer name. > + * We'll copy it to the user space name back when we're done > + */ > + kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); Please sanity-check the amount of memory we try to allocate. > + if (!kname) > + return -ENOMEM; > + > + xpnir.p_name = kname; > + error = xfs_attr_get_parent_pointer(ip, &xpnir); > + > + if (error) > + goto out; > + > + copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); > + xpnir.p_name = uname; > + copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); > + > +out: > + kmem_free(kname); > + return error; > +} > + > int > xfs_ioc_swapext( > xfs_swapext_t *sxp) > @@ -1866,7 +1911,8 @@ xfs_file_ioctl( > return xfs_ioc_getxflags(ip, arg); > case XFS_IOC_SETXFLAGS: > return xfs_ioc_setxflags(ip, filp, arg); > - > + case XFS_IOC_GETPPOINTER: > + return xfs_ioc_get_parent_pointer(filp, arg); > case XFS_IOC_FSSETDM: { > struct fsdmidata dmi; > > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/28/2017 01:35 PM, Darrick J. Wong wrote: > On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: >> This patch adds a new file ioctl to retrieve the parent >> pointer of a given inode >> >> Signed-off-by: Allison Henderson<allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ >> fs/xfs/libxfs/xfs_fs.h | 1 + >> fs/xfs/xfs_attr.h | 2 ++ >> fs/xfs/xfs_attr_list.c | 3 +++ >> fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- >> 5 files changed, 120 insertions(+), 1 deletion(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 9d4d883..d2be842 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( >> return xfs_attr_node_get(args); >> } >> >> +/* >> + * Get the parent pointer for a given inode >> + * Caller will need to allocate a buffer pointed to by xpnir->p_name >> + * and store the buffer size in xpnir->p_namelen. The parent >> + * pointer will be stored in the given xfs_parent_name_irec >> + * >> + * Returns 0 on success and non zero on error >> + */ >> +int >> +xfs_attr_get_parent_pointer(struct xfs_inode *ip, >> + struct xfs_parent_name_irec *xpnir) > Please fix the parameter list here. > >> +{ >> + struct attrlist *alist; >> + struct attrlist_ent *aent; >> + struct attrlist_cursor_kern cursor; >> + struct xfs_parent_name_rec *xpnr; >> + char *namebuf; >> + int error = 0; >> + unsigned int flags = ATTR_PARENT; >> + >> + /* Allocate a buffer to store the attribute names */ >> + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); >> + if (!namebuf) >> + return -ENOMEM; >> + >> + /* Get all attribute names that have the ATTR_PARENT flag */ >> + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); >> + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); >> + if (error) >> + goto out_kfree; >> + >> + alist = (struct attrlist *)namebuf; >> + >> + /* There should never be more than one parent pointer */ >> + ASSERT(alist->al_count == 1); > As mentioned earlier, this is not true. Files can have multiple parents. > >> + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; >> + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); >> + >> + /* >> + * The value of the parent pointer attribute should be the file name >> + * So we check the value length of the attribute entry against the name >> + * length of the parent name record to make sure the caller gave enough >> + * buffer space to store the file name (plus a null terminator) >> + */ >> + if (aent->a_valuelen >= xpnir->p_namelen) { >> + error = -ERANGE; >> + goto out_kfree; >> + } >> + >> + xpnir->p_namelen = aent->a_valuelen + 1; >> + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); >> + error = xfs_attr_get(ip, (char *)xpnr, >> + sizeof(struct xfs_parent_name_rec), >> + (unsigned char *)(xpnir->p_name), >> + (int *)&(xpnir->p_namelen), flags); >> + if (error) >> + goto out_kfree; >> + >> + xfs_init_parent_name_irec(xpnir, xpnr); >> + >> +out_kfree: >> + kmem_free(namebuf); >> + >> + return error; >> +} >> + >> /* Retrieve an extended attribute by name, and its value. */ >> int >> xfs_attr_get( >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h >> index b8108f8..2f9ca2c 100644 >> --- a/fs/xfs/libxfs/xfs_fs.h >> +++ b/fs/xfs/libxfs/xfs_fs.h >> @@ -512,6 +512,7 @@ typedef struct xfs_swapext >> #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) >> #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) >> /* XFS_IOC_GETFSMAP ------ hoisted 59 */ >> +#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) > I don't think it's a good idea to expose internal data structures > directly to userspace, because that inhibits our ability to change the > in-core data structure. Yes, this part I already have that separated in my local copy > Furthermore, hardlinked files can have multiple parent pointers, so it's > not going to suffice to return a single parent pointer entry. Given > that there can be potentially 2^32 parents, we're going to need a data > structure for the ioctl to store (in an opaque manner) the attribute > iteration cursor and have space to pass back some number of parent > pointers. > > (Yes, it's time to start talking about actual use cases...) > > At a bare minimum, this is what I pictured for the "return parents of > the open file" ioctl: > > #define XFS_PPTR_MAXNAMELEN 255 > > struct xfs_pptr { > u64 pp_ino; > u32 pp_gen; > u8 pp_namelen; > u8 pp_name[XFS_PPTR_MAXNAMELEN]; > }; > > /* return parents of the handle, instead of the open fd */ > #define XFS_PPTR_FLAG_HANDLE (1u << 0) > > struct xfs_pptr_info { > struct xfs_fsop_handlereq pi_handle; > struct xfs_attrlist_cursor pi_cursor; > u32 pi_flags; > u32 pi_reserved; > u32 pi_ptrs_size; > u32 pi_ptrs_used; > u64 pi_reserved2[6]; > struct xfs_pptr pi_ptrs[0]; > }; > > #define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ > ((ptrs) * sizeof(struct xfs_pptr))); > > static inline struct xfs_pptr_info * > xfs_pptr_alloc( > size_t nr_ptrs) > { > struct xfs_pptr_info *ppi; > > ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > if (!ppi) > return NULL; > memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > ppi->pi_ptrs_size = nr_ptrs; > return ppi; > } > > With the following example userspace program (that does no checking > whatsoever): > > int main(int argc, char *argv[]) > { > struct xfs_pptr_info *ppi; > struct xfs_pptr *pp; > int fd; > > fd = open(argv[1], O_RDONLY); > ppi = xfs_pptr_alloc(32); > > while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { > for (i = 0; i < ppi->pi_ptrs_used; i++) { > printf("%llu:%u -> %s\n", > ppi->pi_ptrs[i].pp_ino, > ppi->pi_ptrs[i].pp_gen, > ppi->pi_ptrs[i].pp_name); > } > } > } > > Notice here how we the userspace structure contains an opaque attribute > list cursor, so we can keep coming back for more parent pointers until > we run out of xattrs (and pi_ptrs_used == 0). The kernel will copy its > internal cursor out to the userspace buffer as an opaque cookie prior to > returning. > > From this simple implementation it shouldn't be difficult to finish the > parents_by_handle/parentpaths_by_handle functions in libhandle, though > given that they've never been implemented in Linux and we no longer care > about Irix, you've some flexibility to change those library functions if > that is convenient for setting up xfstests. Wow, ok that makes a lot of sense. I will follow your model here and get it fleshed out. Thank you! > Speaking of xfstests... what are the initial test cases? I figured at > least the following: > > 0) mkfs with protofile, make sure the parent records get created > 1) create file, check parent records > 2) hardlink file, check both parent records > 3) delete one link of a hardlinked file, check parent records > 4) hardlink a file a few thousand times, check that the iteration > scheme laid out above actually works > 5) rename a file within a directory, check the parent records > 6) rename a file across directories, check the parent records > 7) some sort of testing where we run out of space while updating pptrs > 8) add some error injection knobs to make sure that pptr replay actually > works correctly > > Can you think of other test cases? I think that is a good start. This looks similar to what I've been doing by hand to stabilize things as I go along. I'll have to work on developing an inject knob for the last one. > For xfs_scrub, we want to be able to query the parents of any (damaged) > inode we find in the filesystem. If the inode is so damaged we can't > open it (or it's a special file) then scrub has to construct a file > handle and pass that in via pi_handle. Alrighty, I will take a look at those routines and see if I can put together something that reconstructs the parent pointers with out opening the inode > I /also/ wonder if there's any interest in having a fallback for > non-pptr filesystems that walks the dentry->d_parent links (like > d_paths() does) back to the root. Such a fallback will only work on an > opened dir or a file opened by path (i.e. not a handle), however, which > limits its appeal. > > --D You mean a way to get the parent pointer even if they chose not to enable the feature flag? I think its something we could investigate, but I think you're right in that the limitations might not make it quite as valuable. IMHO I think maybe getting the full version working first might give people a chance to appreciate what it can do, and if it turns out to be something that people end up using a lot, then it might generate more demand for the "light" version. :-) >> /* >> * ioctl commands that replace IRIX syssgi()'s >> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h >> index 0829687..0ec3458 100644 >> --- a/fs/xfs/xfs_attr.h >> +++ b/fs/xfs/xfs_attr.h >> @@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, >> int flags); >> int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, >> size_t namelen, unsigned char *value, int valuelen, int flags); >> +int xfs_attr_get_parent_pointer(struct xfs_inode *ip, >> + struct xfs_parent_name_irec *xpnir); >> int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); >> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, >> size_t namelen, int flags); >> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c >> index 7740c8a..78fc477 100644 >> --- a/fs/xfs/xfs_attr_list.c >> +++ b/fs/xfs/xfs_attr_list.c >> @@ -534,6 +534,9 @@ xfs_attr_put_listent( >> if (((context->flags & ATTR_ROOT) == 0) != >> ((flags & XFS_ATTR_ROOT) == 0)) >> return; >> + if (((context->flags & ATTR_PARENT) == 0) != >> + ((flags & XFS_ATTR_PARENT) == 0)) >> + return; >> >> arraytop = sizeof(*alist) + >> context->count * sizeof(alist->al_offset[0]); >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 4664314..5492607 100644 >> --- a/fs/xfs/xfs_ioctl.c >> +++ b/fs/xfs/xfs_ioctl.c >> @@ -44,6 +44,7 @@ >> #include "xfs_btree.h" >> #include <linux/fsmap.h> >> #include "xfs_fsmap.h" >> +#include "xfs_attr.h" >> >> #include <linux/capability.h> >> #include <linux/cred.h> >> @@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( >> return 0; >> } >> >> +/* >> + * IOCTL routine to get the parent pointer of an inode and return it to user >> + * space. Caller must pass an struct xfs_parent_name_irec with a name buffer >> + * large enough to hold the file name. Returns 0 on success or non-zero on >> + * failure >> + */ >> +STATIC int >> +xfs_ioc_get_parent_pointer( >> + struct file *filp, >> + void __user *arg) >> +{ >> + struct inode *inode = file_inode(filp); >> + struct xfs_inode *ip = XFS_I(inode); >> + struct xfs_parent_name_irec xpnir; >> + char *uname; >> + char *kname; >> + int error = 0; >> + >> + copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); >> + uname = (char *)xpnir.p_name; >> + >> + /* >> + * Use kernel space memory to get the parent pointer name. >> + * We'll copy it to the user space name back when we're done >> + */ >> + kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); > Please sanity-check the amount of memory we try to allocate. > >> + if (!kname) >> + return -ENOMEM; >> + >> + xpnir.p_name = kname; >> + error = xfs_attr_get_parent_pointer(ip, &xpnir); >> + >> + if (error) >> + goto out; >> + >> + copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); >> + xpnir.p_name = uname; >> + copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); >> + >> +out: >> + kmem_free(kname); >> + return error; >> +} >> + >> int >> xfs_ioc_swapext( >> xfs_swapext_t *sxp) >> @@ -1866,7 +1911,8 @@ xfs_file_ioctl( >> return xfs_ioc_getxflags(ip, arg); >> case XFS_IOC_SETXFLAGS: >> return xfs_ioc_setxflags(ip, filp, arg); >> - >> + case XFS_IOC_GETPPOINTER: >> + return xfs_ioc_get_parent_pointer(filp, arg); >> case XFS_IOC_FSSETDM: { >> struct fsdmidata dmi; >> >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message tomajordomo@vger.kernel.org >> More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=4f7DOEYDfWf_ZRdBfE0cU7L0QfDJjKolv1tc2HeLeck&s=6K6iOFwNgQv30L_9mpWjoAPsnvxojOglPp6hADhWRb8&e= > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message tomajordomo@vger.kernel.org > More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=4f7DOEYDfWf_ZRdBfE0cU7L0QfDJjKolv1tc2HeLeck&s=6K6iOFwNgQv30L_9mpWjoAPsnvxojOglPp6hADhWRb8&e= -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: > On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: > > This patch adds a new file ioctl to retrieve the parent > > pointer of a given inode > > (Yes, it's time to start talking about actual use cases...) > > At a bare minimum, this is what I pictured for the "return parents of > the open file" ioctl: > > #define XFS_PPTR_MAXNAMELEN 255 > > struct xfs_pptr { > u64 pp_ino; > u32 pp_gen; > u8 pp_namelen; > u8 pp_name[XFS_PPTR_MAXNAMELEN]; > }; That's going to be a different size on 32bit and 64 bit platforms as the structure size is a multiple of 4 bytes, not 8 bytes. That will cause problems and need complex comapt ioctl translation. Better to make pp_namelen a u32 and that will make the structure 64 bit aligned and sized on all platforms. I'd allow more than u8 for the namelen. Yes, while we currently allow on 255 bytes for a name, it would make more sense to use a u32 here so that the structure size is a multiple of it's alignment rather than having a 4 byte hole in the array we don't fill out.... > > /* return parents of the handle, instead of the open fd */ > #define XFS_PPTR_FLAG_HANDLE (1u << 0) > > struct xfs_pptr_info { > struct xfs_fsop_handlereq pi_handle; > struct xfs_attrlist_cursor pi_cursor; > u32 pi_flags; > u32 pi_reserved; > u32 pi_ptrs_size; > u32 pi_ptrs_used; > u64 pi_reserved2[6]; > struct xfs_pptr pi_ptrs[0]; > }; I thought gcc had started doing weird things with variable size array declarations like this (i.e. pi_ptrs[0]) because the exact behaviour is not defined in the C standard. i.e. we need to avoid adding new declarations that do this... > #define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ > ((ptrs) * sizeof(struct xfs_pptr))); > static inline struct xfs_pptr_info * > xfs_pptr_alloc( > size_t nr_ptrs) > { > struct xfs_pptr_info *ppi; > > ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > if (!ppi) > return NULL; > memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > ppi->pi_ptrs_size = nr_ptrs; > return ppi; > } > > With the following example userspace program (that does no checking > whatsoever): > > int main(int argc, char *argv[]) > { > struct xfs_pptr_info *ppi; > struct xfs_pptr *pp; > int fd; > > fd = open(argv[1], O_RDONLY); > ppi = xfs_pptr_alloc(32); > > while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { > for (i = 0; i < ppi->pi_ptrs_used; i++) { > printf("%llu:%u -> %s\n", > ppi->pi_ptrs[i].pp_ino, > ppi->pi_ptrs[i].pp_gen, > ppi->pi_ptrs[i].pp_name); > } > } > } Seems like a reasonable model to me. > I /also/ wonder if there's any interest in having a fallback for > non-pptr filesystems that walks the dentry->d_parent links (like > d_paths() does) back to the root. Such a fallback will only work on an > opened dir or a file opened by path (i.e. not a handle), however, which > limits its appeal. I wouldn't bother complicating anything by trying to support filesytems that don't have parent pointer info. Just have non-parent-ptr filesystems return EOPNOTSUPP for the ioctl and be done with it... Cheers, Dave.
On 11/29/2017 02:37 PM, Dave Chinner wrote: > On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: >> On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: >>> This patch adds a new file ioctl to retrieve the parent >>> pointer of a given inode >> >> (Yes, it's time to start talking about actual use cases...) >> >> At a bare minimum, this is what I pictured for the "return parents of >> the open file" ioctl: >> >> #define XFS_PPTR_MAXNAMELEN 255 >> >> struct xfs_pptr { >> u64 pp_ino; >> u32 pp_gen; >> u8 pp_namelen; >> u8 pp_name[XFS_PPTR_MAXNAMELEN]; >> }; > > That's going to be a different size on 32bit and 64 bit platforms > as the structure size is a multiple of 4 bytes, not 8 bytes. > That will cause problems and need complex comapt ioctl translation. > Better to make pp_namelen a u32 and that will make the structure > 64 bit aligned and sized on all platforms. > > I'd allow more than u8 for the namelen. Yes, while we currently > allow on 255 bytes for a name, it would make more sense to > use a u32 here so that the structure size is a multiple of it's > alignment rather than having a 4 byte hole in the array we don't > fill out.... > >> >> /* return parents of the handle, instead of the open fd */ >> #define XFS_PPTR_FLAG_HANDLE (1u << 0) >> >> struct xfs_pptr_info { >> struct xfs_fsop_handlereq pi_handle; >> struct xfs_attrlist_cursor pi_cursor; >> u32 pi_flags; >> u32 pi_reserved; >> u32 pi_ptrs_size; >> u32 pi_ptrs_used; >> u64 pi_reserved2[6]; >> struct xfs_pptr pi_ptrs[0]; >> }; > > I thought gcc had started doing weird things with variable size > array declarations like this (i.e. pi_ptrs[0]) because the exact > behaviour is not defined in the C standard. i.e. we need to avoid > adding new declarations that do this... Oh, I think there's a few places in the set where I have declarations like that. Should they be some_array[1]; instead? > > >> #define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ >> ((ptrs) * sizeof(struct xfs_pptr))); >> static inline struct xfs_pptr_info * >> xfs_pptr_alloc( >> size_t nr_ptrs) >> { >> struct xfs_pptr_info *ppi; >> >> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); >> if (!ppi) >> return NULL; >> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); >> ppi->pi_ptrs_size = nr_ptrs; >> return ppi; >> } >> >> With the following example userspace program (that does no checking >> whatsoever): >> >> int main(int argc, char *argv[]) >> { >> struct xfs_pptr_info *ppi; >> struct xfs_pptr *pp; >> int fd; >> >> fd = open(argv[1], O_RDONLY); >> ppi = xfs_pptr_alloc(32); >> >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { >> for (i = 0; i < ppi->pi_ptrs_used; i++) { >> printf("%llu:%u -> %s\n", >> ppi->pi_ptrs[i].pp_ino, >> ppi->pi_ptrs[i].pp_gen, >> ppi->pi_ptrs[i].pp_name); >> } >> } >> } > > Seems like a reasonable model to me. > >> I /also/ wonder if there's any interest in having a fallback for >> non-pptr filesystems that walks the dentry->d_parent links (like >> d_paths() does) back to the root. Such a fallback will only work on an >> opened dir or a file opened by path (i.e. not a handle), however, which >> limits its appeal. > > I wouldn't bother complicating anything by trying to support > filesytems that don't have parent pointer info. Just have > non-parent-ptr filesystems return EOPNOTSUPP for the ioctl and be > done with it... > > Cheers, > > Dave. > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote: > > > On 11/29/2017 02:37 PM, Dave Chinner wrote: > >On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: > >>On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: > >>>This patch adds a new file ioctl to retrieve the parent > >>>pointer of a given inode > >> > >>(Yes, it's time to start talking about actual use cases...) > >> > >>At a bare minimum, this is what I pictured for the "return parents of > >>the open file" ioctl: > >> > >>#define XFS_PPTR_MAXNAMELEN 255 > >> > >>struct xfs_pptr { > >> u64 pp_ino; > >> u32 pp_gen; > >> u8 pp_namelen; > >> u8 pp_name[XFS_PPTR_MAXNAMELEN]; > >>}; > > > >That's going to be a different size on 32bit and 64 bit platforms > >as the structure size is a multiple of 4 bytes, not 8 bytes. > >That will cause problems and need complex comapt ioctl translation. > >Better to make pp_namelen a u32 and that will make the structure > >64 bit aligned and sized on all platforms. > > > >I'd allow more than u8 for the namelen. Yes, while we currently > >allow on 255 bytes for a name, it would make more sense to > >use a u32 here so that the structure size is a multiple of it's > >alignment rather than having a 4 byte hole in the array we don't > >fill out.... > > > >> > >>/* return parents of the handle, instead of the open fd */ > >>#define XFS_PPTR_FLAG_HANDLE (1u << 0) > >> > >>struct xfs_pptr_info { > >> struct xfs_fsop_handlereq pi_handle; > >> struct xfs_attrlist_cursor pi_cursor; > >> u32 pi_flags; > >> u32 pi_reserved; > >> u32 pi_ptrs_size; > >> u32 pi_ptrs_used; > >> u64 pi_reserved2[6]; > >> struct xfs_pptr pi_ptrs[0]; > >>}; > > > >I thought gcc had started doing weird things with variable size > >array declarations like this (i.e. pi_ptrs[0]) because the exact > >behaviour is not defined in the C standard. i.e. we need to avoid > >adding new declarations that do this... > > Oh, I think there's a few places in the set where I have > declarations like that. Yup, there are quite a few, but IIRC we can't rely on them working as they do right now in future compilers. So I'm pretty sure we need to avoid these sorts of constructs if we can. Doing something like this: struct xfs_pptr_info { struct xfs_fsop_handlereq pi_handle; struct xfs_attrlist_cursor pi_cursor; u32 pi_flags; u32 pi_reserved; u32 pi_ptrs_size; u32 pi_ptrs_used; u64 pi_reserved2[6]; /* * An array of struct xfs_pptr follows the header * information. Use XFS_PPINFO_TO_PP() to access the * parent pointer array entries. */ }; And providing an accessor function: #define XFS_PPINFO_TO_PP(info, idx) \ (&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)])) Will solve the problem. > Should they be some_array[1]; instead? That has problems, too. See, for example, commit ffeecc521302 ("xfs: Fix xfs_attr_leafblock definition"), where gcc completely mangled the code because it thought it could optimise away bits of the structure and code that "weren't used". > >>#define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ > >> ((ptrs) * sizeof(struct xfs_pptr))); > >>static inline struct xfs_pptr_info * > >>xfs_pptr_alloc( > >> size_t nr_ptrs) > >>{ > >> struct xfs_pptr_info *ppi; > >> > >> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > >> if (!ppi) > >> return NULL; > >> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > >> ppi->pi_ptrs_size = nr_ptrs; > >> return ppi; > >>} > >> > >>With the following example userspace program (that does no checking > >>whatsoever): > >> > >>int main(int argc, char *argv[]) > >>{ > >> struct xfs_pptr_info *ppi; > >> struct xfs_pptr *pp; > >> int fd; > >> > >> fd = open(argv[1], O_RDONLY); > >> ppi = xfs_pptr_alloc(32); > >> > >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { > >> for (i = 0; i < ppi->pi_ptrs_used; i++) { > >> printf("%llu:%u -> %s\n", > >> ppi->pi_ptrs[i].pp_ino, > >> ppi->pi_ptrs[i].pp_gen, > >> ppi->pi_ptrs[i].pp_name); And this becomes: for (i = 0; i < ppi->pi_ptrs_used; i++) { pp = XFS_PPINFO_TO_PP(ppi, i); printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen, pp->pp_name); } Cheers, Dave.
On 11/29/2017 05:02 PM, Dave Chinner wrote: > On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote: >> >> >> On 11/29/2017 02:37 PM, Dave Chinner wrote: >>> On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: >>>> On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: >>>>> This patch adds a new file ioctl to retrieve the parent >>>>> pointer of a given inode >>>> >>>> (Yes, it's time to start talking about actual use cases...) >>>> >>>> At a bare minimum, this is what I pictured for the "return parents of >>>> the open file" ioctl: >>>> >>>> #define XFS_PPTR_MAXNAMELEN 255 >>>> >>>> struct xfs_pptr { >>>> u64 pp_ino; >>>> u32 pp_gen; >>>> u8 pp_namelen; >>>> u8 pp_name[XFS_PPTR_MAXNAMELEN]; >>>> }; >>> >>> That's going to be a different size on 32bit and 64 bit platforms >>> as the structure size is a multiple of 4 bytes, not 8 bytes. >>> That will cause problems and need complex comapt ioctl translation. >>> Better to make pp_namelen a u32 and that will make the structure >>> 64 bit aligned and sized on all platforms. >>> >>> I'd allow more than u8 for the namelen. Yes, while we currently >>> allow on 255 bytes for a name, it would make more sense to >>> use a u32 here so that the structure size is a multiple of it's >>> alignment rather than having a 4 byte hole in the array we don't >>> fill out.... >>> >>>> >>>> /* return parents of the handle, instead of the open fd */ >>>> #define XFS_PPTR_FLAG_HANDLE (1u << 0) >>>> >>>> struct xfs_pptr_info { >>>> struct xfs_fsop_handlereq pi_handle; >>>> struct xfs_attrlist_cursor pi_cursor; >>>> u32 pi_flags; >>>> u32 pi_reserved; >>>> u32 pi_ptrs_size; >>>> u32 pi_ptrs_used; >>>> u64 pi_reserved2[6]; >>>> struct xfs_pptr pi_ptrs[0]; >>>> }; >>> >>> I thought gcc had started doing weird things with variable size >>> array declarations like this (i.e. pi_ptrs[0]) because the exact >>> behaviour is not defined in the C standard. i.e. we need to avoid >>> adding new declarations that do this... >> >> Oh, I think there's a few places in the set where I have >> declarations like that. > > Yup, there are quite a few, but IIRC we can't rely on them working > as they do right now in future compilers. So I'm pretty sure we need > to avoid these sorts of constructs if we can. Doing something like > this: > > struct xfs_pptr_info { > struct xfs_fsop_handlereq pi_handle; > struct xfs_attrlist_cursor pi_cursor; > u32 pi_flags; > u32 pi_reserved; > u32 pi_ptrs_size; > u32 pi_ptrs_used; > u64 pi_reserved2[6]; > > /* > * An array of struct xfs_pptr follows the header > * information. Use XFS_PPINFO_TO_PP() to access the > * parent pointer array entries. > */ > }; > > And providing an accessor function: > > #define XFS_PPINFO_TO_PP(info, idx) \ > (&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)])) > > Will solve the problem. > >> Should they be some_array[1]; instead? > > That has problems, too. See, for example, commit ffeecc521302 ("xfs: > Fix xfs_attr_leafblock definition"), where gcc completely mangled > the code because it thought it could optimise away bits of the > structure and code that "weren't used". > >>>> #define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ >>>> ((ptrs) * sizeof(struct xfs_pptr))); >>>> static inline struct xfs_pptr_info * >>>> xfs_pptr_alloc( >>>> size_t nr_ptrs) >>>> { >>>> struct xfs_pptr_info *ppi; >>>> >>>> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); >>>> if (!ppi) >>>> return NULL; >>>> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); >>>> ppi->pi_ptrs_size = nr_ptrs; >>>> return ppi; >>>> } >>>> >>>> With the following example userspace program (that does no checking >>>> whatsoever): >>>> >>>> int main(int argc, char *argv[]) >>>> { >>>> struct xfs_pptr_info *ppi; >>>> struct xfs_pptr *pp; >>>> int fd; >>>> >>>> fd = open(argv[1], O_RDONLY); >>>> ppi = xfs_pptr_alloc(32); >>>> >>>> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { >>>> for (i = 0; i < ppi->pi_ptrs_used; i++) { >>>> printf("%llu:%u -> %s\n", >>>> ppi->pi_ptrs[i].pp_ino, >>>> ppi->pi_ptrs[i].pp_gen, >>>> ppi->pi_ptrs[i].pp_name); > > And this becomes: > > for (i = 0; i < ppi->pi_ptrs_used; i++) { > pp = XFS_PPINFO_TO_PP(ppi, i); > printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen, > pp->pp_name); > } > > Cheers, > > Dave. > Alrighty then, thank you! Allison -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 30, 2017 at 11:02:51AM +1100, Dave Chinner wrote: > On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote: > > > > > > On 11/29/2017 02:37 PM, Dave Chinner wrote: > > >On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: > > >>On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: > > >>>This patch adds a new file ioctl to retrieve the parent > > >>>pointer of a given inode > > >> > > >>(Yes, it's time to start talking about actual use cases...) > > >> > > >>At a bare minimum, this is what I pictured for the "return parents of > > >>the open file" ioctl: > > >> > > >>#define XFS_PPTR_MAXNAMELEN 255 > > >> > > >>struct xfs_pptr { > > >> u64 pp_ino; > > >> u32 pp_gen; > > >> u8 pp_namelen; > > >> u8 pp_name[XFS_PPTR_MAXNAMELEN]; > > >>}; > > > > > >That's going to be a different size on 32bit and 64 bit platforms > > >as the structure size is a multiple of 4 bytes, not 8 bytes. > > >That will cause problems and need complex comapt ioctl translation. > > >Better to make pp_namelen a u32 and that will make the structure > > >64 bit aligned and sized on all platforms. > > > > > >I'd allow more than u8 for the namelen. Yes, while we currently > > >allow on 255 bytes for a name, it would make more sense to > > >use a u32 here so that the structure size is a multiple of it's > > >alignment rather than having a 4 byte hole in the array we don't > > >fill out.... Maybe this ought to get padded up to the nearest 8-byte boundary too. > > > > > >> > > >>/* return parents of the handle, instead of the open fd */ > > >>#define XFS_PPTR_FLAG_HANDLE (1u << 0) > > >> > > >>struct xfs_pptr_info { > > >> struct xfs_fsop_handlereq pi_handle; > > >> struct xfs_attrlist_cursor pi_cursor; > > >> u32 pi_flags; > > >> u32 pi_reserved; > > >> u32 pi_ptrs_size; > > >> u32 pi_ptrs_used; > > >> u64 pi_reserved2[6]; > > >> struct xfs_pptr pi_ptrs[0]; > > >>}; > > > > > >I thought gcc had started doing weird things with variable size > > >array declarations like this (i.e. pi_ptrs[0]) because the exact > > >behaviour is not defined in the C standard. i.e. we need to avoid > > >adding new declarations that do this... > > > > Oh, I think there's a few places in the set where I have > > declarations like that. > > Yup, there are quite a few, but IIRC we can't rely on them working > as they do right now in future compilers. So I'm pretty sure we need > to avoid these sorts of constructs if we can. Doing something like > this: If gcc starts bungling them, there's going to be a lot of stuff in include/uapi/ that breaks. FIEMAP, FSMAP, the weird vfs dedupe ioctl... I think it'll be fine so long as we keep an eye on the structure size in xfs_ondisk.h. If the structure size mutates we'll know because the ioctl will stop working with old userspace and/or we fail the build. Oh but we don't keep an eye on that stuff. Sigh. > struct xfs_pptr_info { > struct xfs_fsop_handlereq pi_handle; > struct xfs_attrlist_cursor pi_cursor; > u32 pi_flags; > u32 pi_reserved; > u32 pi_ptrs_size; > u32 pi_ptrs_used; > u64 pi_reserved2[6]; > > /* > * An array of struct xfs_pptr follows the header > * information. Use XFS_PPINFO_TO_PP() to access the > * parent pointer array entries. > */ > }; > > And providing an accessor function: > > #define XFS_PPINFO_TO_PP(info, idx) \ > (&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)])) Eww, macros. :) static inline struct xfs_pptr * xfs_ppinfo_to_pp( struct xfs_pptr_info *info, unsigned int idx) { return (struct xfs_pptr *)((char *)(info + 1)) + (idx * sizeof(struct xfs_pptr)); } > Will solve the problem. > > > Should they be some_array[1]; instead? > > That has problems, too. See, for example, commit ffeecc521302 ("xfs: > Fix xfs_attr_leafblock definition"), where gcc completely mangled > the code because it thought it could optimise away bits of the > structure and code that "weren't used". Especially no on the some_array[1], that bit us with the v5 AGFL... > > >>#define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \ > > >> ((ptrs) * sizeof(struct xfs_pptr))); > > >>static inline struct xfs_pptr_info * > > >>xfs_pptr_alloc( > > >> size_t nr_ptrs) > > >>{ > > >> struct xfs_pptr_info *ppi; > > >> > > >> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > > >> if (!ppi) > > >> return NULL; > > >> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs)); > > >> ppi->pi_ptrs_size = nr_ptrs; > > >> return ppi; > > >>} > > >> > > >>With the following example userspace program (that does no checking > > >>whatsoever): > > >> > > >>int main(int argc, char *argv[]) > > >>{ > > >> struct xfs_pptr_info *ppi; > > >> struct xfs_pptr *pp; > > >> int fd; > > >> > > >> fd = open(argv[1], O_RDONLY); > > >> ppi = xfs_pptr_alloc(32); > > >> > > >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { > > >> for (i = 0; i < ppi->pi_ptrs_used; i++) { > > >> printf("%llu:%u -> %s\n", > > >> ppi->pi_ptrs[i].pp_ino, > > >> ppi->pi_ptrs[i].pp_gen, > > >> ppi->pi_ptrs[i].pp_name); > > And this becomes: > > for (i = 0; i < ppi->pi_ptrs_used; i++) { > pp = XFS_PPINFO_TO_PP(ppi, i); > printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen, > pp->pp_name); > } Funnily enough I've added more bits to this, maybe I should just send a real RFC patch to the list. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 30, 2017 at 01:11:34PM -0800, Darrick J. Wong wrote: > On Thu, Nov 30, 2017 at 11:02:51AM +1100, Dave Chinner wrote: > > On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote: > > > > > > > > > On 11/29/2017 02:37 PM, Dave Chinner wrote: > > > >On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote: > > > >>On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote: > > > >>>This patch adds a new file ioctl to retrieve the parent > > > >>>pointer of a given inode > > > >> > > > >>(Yes, it's time to start talking about actual use cases...) > > > >> > > > >>At a bare minimum, this is what I pictured for the "return parents of > > > >>the open file" ioctl: > > > >> > > > >>#define XFS_PPTR_MAXNAMELEN 255 > > > >> > > > >>struct xfs_pptr { > > > >> u64 pp_ino; > > > >> u32 pp_gen; > > > >> u8 pp_namelen; > > > >> u8 pp_name[XFS_PPTR_MAXNAMELEN]; > > > >>}; > > > > > > > >That's going to be a different size on 32bit and 64 bit platforms > > > >as the structure size is a multiple of 4 bytes, not 8 bytes. > > > >That will cause problems and need complex comapt ioctl translation. > > > >Better to make pp_namelen a u32 and that will make the structure > > > >64 bit aligned and sized on all platforms. > > > > > > > >I'd allow more than u8 for the namelen. Yes, while we currently > > > >allow on 255 bytes for a name, it would make more sense to > > > >use a u32 here so that the structure size is a multiple of it's > > > >alignment rather than having a 4 byte hole in the array we don't > > > >fill out.... > > Maybe this ought to get padded up to the nearest 8-byte boundary too. > > > > > > > > >> > > > >>/* return parents of the handle, instead of the open fd */ > > > >>#define XFS_PPTR_FLAG_HANDLE (1u << 0) > > > >> > > > >>struct xfs_pptr_info { > > > >> struct xfs_fsop_handlereq pi_handle; > > > >> struct xfs_attrlist_cursor pi_cursor; > > > >> u32 pi_flags; > > > >> u32 pi_reserved; > > > >> u32 pi_ptrs_size; > > > >> u32 pi_ptrs_used; > > > >> u64 pi_reserved2[6]; > > > >> struct xfs_pptr pi_ptrs[0]; > > > >>}; > > > > > > > >I thought gcc had started doing weird things with variable size > > > >array declarations like this (i.e. pi_ptrs[0]) because the exact > > > >behaviour is not defined in the C standard. i.e. we need to avoid > > > >adding new declarations that do this... > > > > > > Oh, I think there's a few places in the set where I have > > > declarations like that. > > > > Yup, there are quite a few, but IIRC we can't rely on them working > > as they do right now in future compilers. So I'm pretty sure we need > > to avoid these sorts of constructs if we can. Doing something like > > this: > > If gcc starts bungling them, there's going to be a lot of stuff in > include/uapi/ that breaks. FIEMAP, FSMAP, the weird vfs dedupe ioctl... Yup, that'd kick up a shit storm. But when it's just XFS code that triggers the problem, the compiler developers don't care that it's worked for 20 years, they just quote chapter and verse: "code that relies on undefined language constructs can be broken at any time by the compiler and we don't care. Fix your code!" So regardless of whatever happens elsewhere, we need to avoid adding no potential problems to persistent structures such as on-disk and ioctl interfaces.... > I think it'll be fine so long as we keep an eye on the structure size > in xfs_ondisk.h. If the structure size mutates we'll know because the > ioctl will stop working with old userspace and/or we fail the build. > > Oh but we don't keep an eye on that stuff. Sigh. Because who would expect entire structure members to be optimised away by the compiler? :/ > > struct xfs_pptr_info { > > struct xfs_fsop_handlereq pi_handle; > > struct xfs_attrlist_cursor pi_cursor; > > u32 pi_flags; > > u32 pi_reserved; > > u32 pi_ptrs_size; > > u32 pi_ptrs_used; > > u64 pi_reserved2[6]; > > > > /* > > * An array of struct xfs_pptr follows the header > > * information. Use XFS_PPINFO_TO_PP() to access the > > * parent pointer array entries. > > */ > > }; > > > > And providing an accessor function: > > > > #define XFS_PPINFO_TO_PP(info, idx) \ > > (&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)])) > > Eww, macros. :) You did it first with XFS_PPTR_INFO_SIZEOF() :P > > > >>With the following example userspace program (that does no checking > > > >>whatsoever): > > > >> > > > >>int main(int argc, char *argv[]) > > > >>{ > > > >> struct xfs_pptr_info *ppi; > > > >> struct xfs_pptr *pp; > > > >> int fd; > > > >> > > > >> fd = open(argv[1], O_RDONLY); > > > >> ppi = xfs_pptr_alloc(32); > > > >> > > > >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) { > > > >> for (i = 0; i < ppi->pi_ptrs_used; i++) { > > > >> printf("%llu:%u -> %s\n", > > > >> ppi->pi_ptrs[i].pp_ino, > > > >> ppi->pi_ptrs[i].pp_gen, > > > >> ppi->pi_ptrs[i].pp_name); > > > > And this becomes: > > > > for (i = 0; i < ppi->pi_ptrs_used; i++) { > > pp = XFS_PPINFO_TO_PP(ppi, i); > > printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen, > > pp->pp_name); > > } > > Funnily enough I've added more bits to this, maybe I should just send a > real RFC patch to the list. Sounds like a plan :P Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 9d4d883..d2be842 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -134,6 +134,73 @@ xfs_attr_get_ilocked( return xfs_attr_node_get(args); } +/* + * Get the parent pointer for a given inode + * Caller will need to allocate a buffer pointed to by xpnir->p_name + * and store the buffer size in xpnir->p_namelen. The parent + * pointer will be stored in the given xfs_parent_name_irec + * + * Returns 0 on success and non zero on error + */ +int +xfs_attr_get_parent_pointer(struct xfs_inode *ip, + struct xfs_parent_name_irec *xpnir) +{ + struct attrlist *alist; + struct attrlist_ent *aent; + struct attrlist_cursor_kern cursor; + struct xfs_parent_name_rec *xpnr; + char *namebuf; + int error = 0; + unsigned int flags = ATTR_PARENT; + + /* Allocate a buffer to store the attribute names */ + namebuf = kmem_zalloc_large(XFS_XATTR_LIST_MAX, KM_SLEEP); + if (!namebuf) + return -ENOMEM; + + /* Get all attribute names that have the ATTR_PARENT flag */ + memset(&cursor, 0, sizeof(struct attrlist_cursor_kern)); + error = xfs_attr_list(ip, namebuf, XFS_XATTR_LIST_MAX, flags, &cursor); + if (error) + goto out_kfree; + + alist = (struct attrlist *)namebuf; + + /* There should never be more than one parent pointer */ + ASSERT(alist->al_count == 1); + + aent = (struct attrlist_ent *) &namebuf[alist->al_offset[0]]; + xpnr = (struct xfs_parent_name_rec *)(aent->a_name); + + /* + * The value of the parent pointer attribute should be the file name + * So we check the value length of the attribute entry against the name + * length of the parent name record to make sure the caller gave enough + * buffer space to store the file name (plus a null terminator) + */ + if (aent->a_valuelen >= xpnir->p_namelen) { + error = -ERANGE; + goto out_kfree; + } + + xpnir->p_namelen = aent->a_valuelen + 1; + memset((void *)(xpnir->p_name), 0, xpnir->p_namelen); + error = xfs_attr_get(ip, (char *)xpnr, + sizeof(struct xfs_parent_name_rec), + (unsigned char *)(xpnir->p_name), + (int *)&(xpnir->p_namelen), flags); + if (error) + goto out_kfree; + + xfs_init_parent_name_irec(xpnir, xpnr); + +out_kfree: + kmem_free(namebuf); + + return error; +} + /* Retrieve an extended attribute by name, and its value. */ int xfs_attr_get( diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h index b8108f8..2f9ca2c 100644 --- a/fs/xfs/libxfs/xfs_fs.h +++ b/fs/xfs/libxfs/xfs_fs.h @@ -512,6 +512,7 @@ typedef struct xfs_swapext #define XFS_IOC_ZERO_RANGE _IOW ('X', 57, struct xfs_flock64) #define XFS_IOC_FREE_EOFBLOCKS _IOR ('X', 58, struct xfs_fs_eofblocks) /* XFS_IOC_GETFSMAP ------ hoisted 59 */ +#define XFS_IOC_GETPPOINTER _IOR ('X', 61, struct xfs_parent_name_irec) /* * ioctl commands that replace IRIX syssgi()'s diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h index 0829687..0ec3458 100644 --- a/fs/xfs/xfs_attr.h +++ b/fs/xfs/xfs_attr.h @@ -172,6 +172,8 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, int flags); int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, size_t namelen, unsigned char *value, int valuelen, int flags); +int xfs_attr_get_parent_pointer(struct xfs_inode *ip, + struct xfs_parent_name_irec *xpnir); int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, size_t namelen, int flags); diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c index 7740c8a..78fc477 100644 --- a/fs/xfs/xfs_attr_list.c +++ b/fs/xfs/xfs_attr_list.c @@ -534,6 +534,9 @@ xfs_attr_put_listent( if (((context->flags & ATTR_ROOT) == 0) != ((flags & XFS_ATTR_ROOT) == 0)) return; + if (((context->flags & ATTR_PARENT) == 0) != + ((flags & XFS_ATTR_PARENT) == 0)) + return; arraytop = sizeof(*alist) + context->count * sizeof(alist->al_offset[0]); diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 4664314..5492607 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -44,6 +44,7 @@ #include "xfs_btree.h" #include <linux/fsmap.h> #include "xfs_fsmap.h" +#include "xfs_attr.h" #include <linux/capability.h> #include <linux/cred.h> @@ -1710,6 +1711,50 @@ xfs_ioc_getfsmap( return 0; } +/* + * IOCTL routine to get the parent pointer of an inode and return it to user + * space. Caller must pass an struct xfs_parent_name_irec with a name buffer + * large enough to hold the file name. Returns 0 on success or non-zero on + * failure + */ +STATIC int +xfs_ioc_get_parent_pointer( + struct file *filp, + void __user *arg) +{ + struct inode *inode = file_inode(filp); + struct xfs_inode *ip = XFS_I(inode); + struct xfs_parent_name_irec xpnir; + char *uname; + char *kname; + int error = 0; + + copy_from_user(&xpnir, arg, sizeof(struct xfs_parent_name_irec)); + uname = (char *)xpnir.p_name; + + /* + * Use kernel space memory to get the parent pointer name. + * We'll copy it to the user space name back when we're done + */ + kname = kmem_zalloc_large(xpnir.p_namelen, KM_SLEEP); + if (!kname) + return -ENOMEM; + + xpnir.p_name = kname; + error = xfs_attr_get_parent_pointer(ip, &xpnir); + + if (error) + goto out; + + copy_to_user(uname, xpnir.p_name, xpnir.p_namelen); + xpnir.p_name = uname; + copy_to_user(arg, &xpnir, sizeof(struct xfs_parent_name_irec)); + +out: + kmem_free(kname); + return error; +} + int xfs_ioc_swapext( xfs_swapext_t *sxp) @@ -1866,7 +1911,8 @@ xfs_file_ioctl( return xfs_ioc_getxflags(ip, arg); case XFS_IOC_SETXFLAGS: return xfs_ioc_setxflags(ip, filp, arg); - + case XFS_IOC_GETPPOINTER: + return xfs_ioc_get_parent_pointer(filp, arg); case XFS_IOC_FSSETDM: { struct fsdmidata dmi;
This patch adds a new file ioctl to retrieve the parent pointer of a given inode Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/libxfs/xfs_fs.h | 1 + fs/xfs/xfs_attr.h | 2 ++ fs/xfs/xfs_attr_list.c | 3 +++ fs/xfs/xfs_ioctl.c | 48 +++++++++++++++++++++++++++++++++- 5 files changed, 120 insertions(+), 1 deletion(-)