[PATCH 0/3] Minor orangefs xattr cleanups

[PATCH 0/3] Minor orangefs xattr cleanups

[Andreas Gruenbacher]

Mike,

here are three patches that clean up small glitches in orangefs xattrs.  Please
merge.

Thanks,
Andreas

Andreas Gruenbacher (3):
  orangefs: Remove useless defines
  orangefs: Remove redundant "trusted." xattr handler
  orangefs: Remove useless xattr prefix arguments

 fs/orangefs/acl.c             |  17 +++---
 fs/orangefs/file.c            |   2 -
 fs/orangefs/orangefs-kernel.h |  13 -----
 fs/orangefs/xattr.c           | 124 +++++++++---------------------------------
 4 files changed, 34 insertions(+), 122 deletions(-)

-- 
2.5.5

[PATCH 1/3] orangefs: Remove useless defines

[Andreas Gruenbacher]

The ORANGEFS_XATTR_INDEX_ defines are unused; the ORANGEFS_XATTR_NAME_
defines only obfuscate the code.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/orangefs/acl.c             |  8 ++++----
 fs/orangefs/file.c            |  4 ++--
 fs/orangefs/orangefs-kernel.h | 11 -----------
 fs/orangefs/xattr.c           | 17 ++++++-----------
 4 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
index 03f89db..df24864 100644
--- a/fs/orangefs/acl.c
+++ b/fs/orangefs/acl.c
@@ -18,10 +18,10 @@ struct posix_acl *orangefs_get_acl(struct inode *inode, int type)
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
-		key = ORANGEFS_XATTR_NAME_ACL_ACCESS;
+		key = XATTR_NAME_POSIX_ACL_ACCESS;
 		break;
 	case ACL_TYPE_DEFAULT:
-		key = ORANGEFS_XATTR_NAME_ACL_DEFAULT;
+		key = XATTR_NAME_POSIX_ACL_DEFAULT;
 		break;
 	default:
 		gossip_err("orangefs_get_acl: bogus value of type %d\n", type);
@@ -74,7 +74,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
-		name = ORANGEFS_XATTR_NAME_ACL_ACCESS;
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
 			umode_t mode = inode->i_mode;
 			/*
@@ -98,7 +98,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		}
 		break;
 	case ACL_TYPE_DEFAULT:
-		name = ORANGEFS_XATTR_NAME_ACL_DEFAULT;
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
 		break;
 	default:
 		gossip_err("%s: invalid type %d!\n", __func__, type);
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index 491e82c..5160a3f 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -516,7 +516,7 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (cmd == FS_IOC_GETFLAGS) {
 		val = 0;
 		ret = orangefs_inode_getxattr(file_inode(file),
-					      ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+					      "",
 					      "user.pvfs2.meta_hint",
 					      &val, sizeof(val));
 		if (ret < 0 && ret != -ENODATA)
@@ -549,7 +549,7 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 			     "orangefs_ioctl: FS_IOC_SETFLAGS: %llu\n",
 			     (unsigned long long)val);
 		ret = orangefs_inode_setxattr(file_inode(file),
-					      ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+					      "",
 					      "user.pvfs2.meta_hint",
 					      &val, sizeof(val), 0);
 	}
diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
index 2281882..6503e37 100644
--- a/fs/orangefs/orangefs-kernel.h
+++ b/fs/orangefs/orangefs-kernel.h
@@ -119,17 +119,6 @@ struct client_debug_mask {
 #define ORANGEFS_CACHE_CREATE_FLAGS 0
 #endif /* ((defined ORANGEFS_KERNEL_DEBUG) && (defined CONFIG_DEBUG_SLAB)) */
 
-/* orangefs xattr and acl related defines */
-#define ORANGEFS_XATTR_INDEX_POSIX_ACL_ACCESS  1
-#define ORANGEFS_XATTR_INDEX_POSIX_ACL_DEFAULT 2
-#define ORANGEFS_XATTR_INDEX_TRUSTED           3
-#define ORANGEFS_XATTR_INDEX_DEFAULT           4
-
-#define ORANGEFS_XATTR_NAME_ACL_ACCESS XATTR_NAME_POSIX_ACL_ACCESS
-#define ORANGEFS_XATTR_NAME_ACL_DEFAULT XATTR_NAME_POSIX_ACL_DEFAULT
-#define ORANGEFS_XATTR_NAME_TRUSTED_PREFIX "trusted."
-#define ORANGEFS_XATTR_NAME_DEFAULT_PREFIX ""
-
 /* these functions are defined in orangefs-utils.c */
 int orangefs_prepare_cdm_array(char *debug_array_string);
 int orangefs_prepare_debugfs_help_string(int);
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 99c1954..2944d0a 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -455,7 +455,7 @@ static int orangefs_xattr_set_default(const struct xattr_handler *handler,
 				      int flags)
 {
 	return orangefs_inode_setxattr(dentry->d_inode,
-				    ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+				    "",
 				    name,
 				    buffer,
 				    size,
@@ -470,7 +470,7 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
 				      size_t size)
 {
 	return orangefs_inode_getxattr(inode,
-				    ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+				    "",
 				    name,
 				    buffer,
 				    size);
@@ -485,7 +485,7 @@ static int orangefs_xattr_set_trusted(const struct xattr_handler *handler,
 				     int flags)
 {
 	return orangefs_inode_setxattr(dentry->d_inode,
-				    ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
+				    XATTR_TRUSTED_PREFIX,
 				    name,
 				    buffer,
 				    size,
@@ -500,25 +500,20 @@ static int orangefs_xattr_get_trusted(const struct xattr_handler *handler,
 				      size_t size)
 {
 	return orangefs_inode_getxattr(inode,
-				    ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
+				    XATTR_TRUSTED_PREFIX,
 				    name,
 				    buffer,
 				    size);
 }
 
 static struct xattr_handler orangefs_xattr_trusted_handler = {
-	.prefix = ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
+	.prefix = XATTR_TRUSTED_PREFIX,
 	.get = orangefs_xattr_get_trusted,
 	.set = orangefs_xattr_set_trusted,
 };
 
 static struct xattr_handler orangefs_xattr_default_handler = {
-	/*
-	 * NOTE: this is set to be the empty string.
-	 * so that all un-prefixed xattrs keys get caught
-	 * here!
-	 */
-	.prefix = ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+	.prefix = "",  /* match any name => handlers called with full name */
 	.get = orangefs_xattr_get_default,
 	.set = orangefs_xattr_set_default,
 };
-- 
2.5.5

Re: [PATCH 1/3] orangefs: Remove useless defines

[Mike Marshall]

Hi Andreas...

I've looked over your patch series, and was about
to apply it and run some tests... but the first patch
"orangefs: Remove useless defines" won't apply.

I received your patch series a day or so after I merged
our kernel.org tree up to 4.6, and at first I thought maybe
you'd composed your patch during the long spell when
the last commit in our tree was e56f49814250f4ca4b66ec7d3a71152846761d1b,
resulting in some kind of conflict, but that's not it. Now
that we're upstream, you're probably patching against
the "real" Linux tree anyhow...

Here's where the fail happens:

@@ -470,7 +470,7 @@ static int orangefs_xattr_get_default(const struct
xattr_handler *handler,
                                      size_t size)
 {
        return orangefs_inode_getxattr(inode,
-                                   ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
+                                   "",
                                    name,
                                    buffer,
                                    size);

"return orangefs_inode_getxattr(inode," should be
"return orangefs_inode_getxattr(dentry->d_inode,"

I looked way back in time, and don't think it ever
was "inode" instead of "dentry->d_inode", or how
it could ever be.

I'm confused.

-Mike "again <g> "

On Wed, May 25, 2016 at 6:29 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> The ORANGEFS_XATTR_INDEX_ defines are unused; the ORANGEFS_XATTR_NAME_
> defines only obfuscate the code.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/orangefs/acl.c             |  8 ++++----
>  fs/orangefs/file.c            |  4 ++--
>  fs/orangefs/orangefs-kernel.h | 11 -----------
>  fs/orangefs/xattr.c           | 17 ++++++-----------
>  4 files changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
> index 03f89db..df24864 100644
> --- a/fs/orangefs/acl.c
> +++ b/fs/orangefs/acl.c
> @@ -18,10 +18,10 @@ struct posix_acl *orangefs_get_acl(struct inode *inode, int type)
>
>         switch (type) {
>         case ACL_TYPE_ACCESS:
> -               key = ORANGEFS_XATTR_NAME_ACL_ACCESS;
> +               key = XATTR_NAME_POSIX_ACL_ACCESS;
>                 break;
>         case ACL_TYPE_DEFAULT:
> -               key = ORANGEFS_XATTR_NAME_ACL_DEFAULT;
> +               key = XATTR_NAME_POSIX_ACL_DEFAULT;
>                 break;
>         default:
>                 gossip_err("orangefs_get_acl: bogus value of type %d\n", type);
> @@ -74,7 +74,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>
>         switch (type) {
>         case ACL_TYPE_ACCESS:
> -               name = ORANGEFS_XATTR_NAME_ACL_ACCESS;
> +               name = XATTR_NAME_POSIX_ACL_ACCESS;
>                 if (acl) {
>                         umode_t mode = inode->i_mode;
>                         /*
> @@ -98,7 +98,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>                 }
>                 break;
>         case ACL_TYPE_DEFAULT:
> -               name = ORANGEFS_XATTR_NAME_ACL_DEFAULT;
> +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
>                 break;
>         default:
>                 gossip_err("%s: invalid type %d!\n", __func__, type);
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index 491e82c..5160a3f 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -516,7 +516,7 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>         if (cmd == FS_IOC_GETFLAGS) {
>                 val = 0;
>                 ret = orangefs_inode_getxattr(file_inode(file),
> -                                             ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +                                             "",
>                                               "user.pvfs2.meta_hint",
>                                               &val, sizeof(val));
>                 if (ret < 0 && ret != -ENODATA)
> @@ -549,7 +549,7 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>                              "orangefs_ioctl: FS_IOC_SETFLAGS: %llu\n",
>                              (unsigned long long)val);
>                 ret = orangefs_inode_setxattr(file_inode(file),
> -                                             ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +                                             "",
>                                               "user.pvfs2.meta_hint",
>                                               &val, sizeof(val), 0);
>         }
> diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
> index 2281882..6503e37 100644
> --- a/fs/orangefs/orangefs-kernel.h
> +++ b/fs/orangefs/orangefs-kernel.h
> @@ -119,17 +119,6 @@ struct client_debug_mask {
>  #define ORANGEFS_CACHE_CREATE_FLAGS 0
>  #endif /* ((defined ORANGEFS_KERNEL_DEBUG) && (defined CONFIG_DEBUG_SLAB)) */
>
> -/* orangefs xattr and acl related defines */
> -#define ORANGEFS_XATTR_INDEX_POSIX_ACL_ACCESS  1
> -#define ORANGEFS_XATTR_INDEX_POSIX_ACL_DEFAULT 2
> -#define ORANGEFS_XATTR_INDEX_TRUSTED           3
> -#define ORANGEFS_XATTR_INDEX_DEFAULT           4
> -
> -#define ORANGEFS_XATTR_NAME_ACL_ACCESS XATTR_NAME_POSIX_ACL_ACCESS
> -#define ORANGEFS_XATTR_NAME_ACL_DEFAULT XATTR_NAME_POSIX_ACL_DEFAULT
> -#define ORANGEFS_XATTR_NAME_TRUSTED_PREFIX "trusted."
> -#define ORANGEFS_XATTR_NAME_DEFAULT_PREFIX ""
> -
>  /* these functions are defined in orangefs-utils.c */
>  int orangefs_prepare_cdm_array(char *debug_array_string);
>  int orangefs_prepare_debugfs_help_string(int);
> diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
> index 99c1954..2944d0a 100644
> --- a/fs/orangefs/xattr.c
> +++ b/fs/orangefs/xattr.c
> @@ -455,7 +455,7 @@ static int orangefs_xattr_set_default(const struct xattr_handler *handler,
>                                       int flags)
>  {
>         return orangefs_inode_setxattr(dentry->d_inode,
> -                                   ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +                                   "",
>                                     name,
>                                     buffer,
>                                     size,
> @@ -470,7 +470,7 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
>                                       size_t size)
>  {
>         return orangefs_inode_getxattr(inode,
> -                                   ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +                                   "",
>                                     name,
>                                     buffer,
>                                     size);
> @@ -485,7 +485,7 @@ static int orangefs_xattr_set_trusted(const struct xattr_handler *handler,
>                                      int flags)
>  {
>         return orangefs_inode_setxattr(dentry->d_inode,
> -                                   ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
> +                                   XATTR_TRUSTED_PREFIX,
>                                     name,
>                                     buffer,
>                                     size,
> @@ -500,25 +500,20 @@ static int orangefs_xattr_get_trusted(const struct xattr_handler *handler,
>                                       size_t size)
>  {
>         return orangefs_inode_getxattr(inode,
> -                                   ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
> +                                   XATTR_TRUSTED_PREFIX,
>                                     name,
>                                     buffer,
>                                     size);
>  }
>
>  static struct xattr_handler orangefs_xattr_trusted_handler = {
> -       .prefix = ORANGEFS_XATTR_NAME_TRUSTED_PREFIX,
> +       .prefix = XATTR_TRUSTED_PREFIX,
>         .get = orangefs_xattr_get_trusted,
>         .set = orangefs_xattr_set_trusted,
>  };
>
>  static struct xattr_handler orangefs_xattr_default_handler = {
> -       /*
> -        * NOTE: this is set to be the empty string.
> -        * so that all un-prefixed xattrs keys get caught
> -        * here!
> -        */
> -       .prefix = ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +       .prefix = "",  /* match any name => handlers called with full name */
>         .get = orangefs_xattr_get_default,
>         .set = orangefs_xattr_set_default,
>  };
> --
> 2.5.5
>

Re: [PATCH 1/3] orangefs: Remove useless defines

[Al Viro]

On Thu, May 26, 2016 at 03:44:23PM -0400, Mike Marshall wrote:
> Hi Andreas...
> 
> I've looked over your patch series, and was about
> to apply it and run some tests... but the first patch
> "orangefs: Remove useless defines" won't apply.
> 
> I received your patch series a day or so after I merged
> our kernel.org tree up to 4.6, and at first I thought maybe
> you'd composed your patch during the long spell when
> the last commit in our tree was e56f49814250f4ca4b66ec7d3a71152846761d1b,
> resulting in some kind of conflict, but that's not it. Now
> that we're upstream, you're probably patching against
> the "real" Linux tree anyhow...
> 
> Here's where the fail happens:
> 
> @@ -470,7 +470,7 @@ static int orangefs_xattr_get_default(const struct
> xattr_handler *handler,
>                                       size_t size)
>  {
>         return orangefs_inode_getxattr(inode,
> -                                   ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
> +                                   "",
>                                     name,
>                                     buffer,
>                                     size);
> 
> "return orangefs_inode_getxattr(inode," should be
> "return orangefs_inode_getxattr(dentry->d_inode,"
> 
> I looked way back in time, and don't think it ever
> was "inode" instead of "dentry->d_inode", or how
> it could ever be.

Mainline has ->getxattr() called with dentry and inode passed separately;
moreover, dentry is *NOT* guaranteed to be already associated with the
inode.  See commit ce23e64 and, subsequently, b96809.

Re: [PATCH 1/3] orangefs: Remove useless defines

[Mike Marshall]

OK... I see now... ce23e64 (Al's patch) got merged to mainline after
2dcd0af (Linux 4.6) and Andreas' Orangefs patches won't load
without ce23e64. I was working from vanilla Linux 4.6. My little
O_DIRECT patch is also affected, since .direct_IO lost the offset
argument.

So... now  the top of my local tree is ea8ea73, Andreas' patch
series loads fine, I've fixed my O_DIRECT patch, it loads, and
the whole thing compiles and installs great. But won't boot. On my VM.
Even when built without Andreas' patch series and my O_DIRECT patch
it won't boot. So now I've got it booted with the vanilla 4.6 kernel.

I guess this is a "tree of the moment" problem. I guess Linus' tree
will continue to evolve and I'll come in tomorrow
and fetch from it again and it will boot. And then I can load
and test the patches, update the Orangefs linux-next and
build the pull request before the merge window ends.

Maybe after six or eight more merge windows I'll get the
hang of being upstream and quit causing trouble <g>...

-Mike



On Thu, May 26, 2016 at 4:05 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, May 26, 2016 at 03:44:23PM -0400, Mike Marshall wrote:
>> Hi Andreas...
>>
>> I've looked over your patch series, and was about
>> to apply it and run some tests... but the first patch
>> "orangefs: Remove useless defines" won't apply.
>>
>> I received your patch series a day or so after I merged
>> our kernel.org tree up to 4.6, and at first I thought maybe
>> you'd composed your patch during the long spell when
>> the last commit in our tree was e56f49814250f4ca4b66ec7d3a71152846761d1b,
>> resulting in some kind of conflict, but that's not it. Now
>> that we're upstream, you're probably patching against
>> the "real" Linux tree anyhow...
>>
>> Here's where the fail happens:
>>
>> @@ -470,7 +470,7 @@ static int orangefs_xattr_get_default(const struct
>> xattr_handler *handler,
>>                                       size_t size)
>>  {
>>         return orangefs_inode_getxattr(inode,
>> -                                   ORANGEFS_XATTR_NAME_DEFAULT_PREFIX,
>> +                                   "",
>>                                     name,
>>                                     buffer,
>>                                     size);
>>
>> "return orangefs_inode_getxattr(inode," should be
>> "return orangefs_inode_getxattr(dentry->d_inode,"
>>
>> I looked way back in time, and don't think it ever
>> was "inode" instead of "dentry->d_inode", or how
>> it could ever be.
>
> Mainline has ->getxattr() called with dentry and inode passed separately;
> moreover, dentry is *NOT* guaranteed to be already associated with the
> inode.  See commit ce23e64 and, subsequently, b96809.

Re: [PATCH 1/3] orangefs: Remove useless defines

[Al Viro]

On Thu, May 26, 2016 at 06:24:22PM -0400, Mike Marshall wrote:
> OK... I see now... ce23e64 (Al's patch) got merged to mainline after
> 2dcd0af (Linux 4.6) and Andreas' Orangefs patches won't load
> without ce23e64. I was working from vanilla Linux 4.6. My little
> O_DIRECT patch is also affected, since .direct_IO lost the offset
> argument.
> 
> So... now  the top of my local tree is ea8ea73, Andreas' patch
> series loads fine, I've fixed my O_DIRECT patch, it loads, and
> the whole thing compiles and installs great. But won't boot. On my VM.
> Even when built without Andreas' patch series and my O_DIRECT patch
> it won't boot. So now I've got it booted with the vanilla 4.6 kernel.
> 
> I guess this is a "tree of the moment" problem. I guess Linus' tree
> will continue to evolve and I'll come in tomorrow
> and fetch from it again and it will boot. And then I can load
> and test the patches, update the Orangefs linux-next and
> build the pull request before the merge window ends.
> 
> Maybe after six or eight more merge windows I'll get the
> hang of being upstream and quit causing trouble <g>...

Useful tip: run the tests you care about on linux-next.  On a regular
basis.  And have your tree in the mix, while we are at it...

Does linux-next circa the beginning of the merge window work for you?
If it doesn't, that's the starting point for git bisect you should've
run back then (or, better yet, at the time when the breakage in linux-next
only started - shorter bisect that way).

Re: [PATCH 1/3] orangefs: Remove useless defines

[Mike Marshall]

OK, so now that there's a problem with b96809 that y'all
are scrambling to fix, it doesn't seem like my little O_DIRECT
patch and Andreas' Orangefs patch series are of
the highest priority.

But... I set up another VM for linux-next testing, and linux-next
won't boot on my VM either.

So I git bisected the problem down to c3bc26d, and I'm
running, booted, with 9222aa8. I started bisecting on the
mainline while I was setting up my linux-next VM, and got
done with that bisect first, so it is mainline, not linux-next,
that I have bisected here.

So... c3bc26d is:
ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support in
acpi_hw_read()
and I have CC'd the Author,  Lv, with this message.

I guess I have some combination in my .config or something
that makes this problem show up for me, surely I'm not the
only one on the Internet trying to boot linux-next and mainline <g>...

Here is a link to my config file and notes (including
git bisect log) I took while doing this bisect.

http://myweb.clemson.edu/~hubcap/bisect.c3bc26d/

-Mike

On Thu, May 26, 2016 at 7:53 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, May 26, 2016 at 06:24:22PM -0400, Mike Marshall wrote:
>> OK... I see now... ce23e64 (Al's patch) got merged to mainline after
>> 2dcd0af (Linux 4.6) and Andreas' Orangefs patches won't load
>> without ce23e64. I was working from vanilla Linux 4.6. My little
>> O_DIRECT patch is also affected, since .direct_IO lost the offset
>> argument.
>>
>> So... now  the top of my local tree is ea8ea73, Andreas' patch
>> series loads fine, I've fixed my O_DIRECT patch, it loads, and
>> the whole thing compiles and installs great. But won't boot. On my VM.
>> Even when built without Andreas' patch series and my O_DIRECT patch
>> it won't boot. So now I've got it booted with the vanilla 4.6 kernel.
>>
>> I guess this is a "tree of the moment" problem. I guess Linus' tree
>> will continue to evolve and I'll come in tomorrow
>> and fetch from it again and it will boot. And then I can load
>> and test the patches, update the Orangefs linux-next and
>> build the pull request before the merge window ends.
>>
>> Maybe after six or eight more merge windows I'll get the
>> hang of being upstream and quit causing trouble <g>...
>
> Useful tip: run the tests you care about on linux-next.  On a regular
> basis.  And have your tree in the mix, while we are at it...
>
> Does linux-next circa the beginning of the merge window work for you?
> If it doesn't, that's the starting point for git bisect you should've
> run back then (or, better yet, at the time when the breakage in linux-next
> only started - shorter bisect that way).

Re: [PATCH 1/3] orangefs: Remove useless defines

[Mike Marshall]

Whatever it is about c3bc26d that makes the VMs on
my work computer ill doesn't bother the VMs on my
home computer, so I can boot linux-next and mainline here
at home. At work my host computer is running Centos-7,
I hope the problem there isn't old QEMU stuff - I guess
plenty of y'all use QEMU VMs hosted on Centos-7?

But the fix to the b96809 problem has changed the
arguments to xattr handlers in mainline, so
Andreas' Orangefs patches no longer apply.

Andreas', when this settles down, would you be
willing to redo and resend your patches?

-Mike

On Fri, May 27, 2016 at 5:24 PM, Mike Marshall <hubcap@omnibond.com> wrote:
> OK, so now that there's a problem with b96809 that y'all
> are scrambling to fix, it doesn't seem like my little O_DIRECT
> patch and Andreas' Orangefs patch series are of
> the highest priority.
>
> But... I set up another VM for linux-next testing, and linux-next
> won't boot on my VM either.
>
> So I git bisected the problem down to c3bc26d, and I'm
> running, booted, with 9222aa8. I started bisecting on the
> mainline while I was setting up my linux-next VM, and got
> done with that bisect first, so it is mainline, not linux-next,
> that I have bisected here.
>
> So... c3bc26d is:
> ACPICA: ACPI 2.0, Hardware: Add access_width/bit_offset support in
> acpi_hw_read()
> and I have CC'd the Author,  Lv, with this message.
>
> I guess I have some combination in my .config or something
> that makes this problem show up for me, surely I'm not the
> only one on the Internet trying to boot linux-next and mainline <g>...
>
> Here is a link to my config file and notes (including
> git bisect log) I took while doing this bisect.
>
> http://myweb.clemson.edu/~hubcap/bisect.c3bc26d/
>
> -Mike
>
> On Thu, May 26, 2016 at 7:53 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Thu, May 26, 2016 at 06:24:22PM -0400, Mike Marshall wrote:
>>> OK... I see now... ce23e64 (Al's patch) got merged to mainline after
>>> 2dcd0af (Linux 4.6) and Andreas' Orangefs patches won't load
>>> without ce23e64. I was working from vanilla Linux 4.6. My little
>>> O_DIRECT patch is also affected, since .direct_IO lost the offset
>>> argument.
>>>
>>> So... now  the top of my local tree is ea8ea73, Andreas' patch
>>> series loads fine, I've fixed my O_DIRECT patch, it loads, and
>>> the whole thing compiles and installs great. But won't boot. On my VM.
>>> Even when built without Andreas' patch series and my O_DIRECT patch
>>> it won't boot. So now I've got it booted with the vanilla 4.6 kernel.
>>>
>>> I guess this is a "tree of the moment" problem. I guess Linus' tree
>>> will continue to evolve and I'll come in tomorrow
>>> and fetch from it again and it will boot. And then I can load
>>> and test the patches, update the Orangefs linux-next and
>>> build the pull request before the merge window ends.
>>>
>>> Maybe after six or eight more merge windows I'll get the
>>> hang of being upstream and quit causing trouble <g>...
>>
>> Useful tip: run the tests you care about on linux-next.  On a regular
>> basis.  And have your tree in the mix, while we are at it...
>>
>> Does linux-next circa the beginning of the merge window work for you?
>> If it doesn't, that's the starting point for git bisect you should've
>> run back then (or, better yet, at the time when the breakage in linux-next
>> only started - shorter bisect that way).

[PATCH 2/3] orangefs: Remove redundant "trusted." xattr handler

[Andreas Gruenbacher]

Orangefs has a catch-all xattr handler that effectively does what the
trusted handler does already.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/orangefs/xattr.c | 36 ------------------------------------
 1 file changed, 36 deletions(-)

diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 2944d0a..1d8be91 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -477,41 +477,6 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
 
 }
 
-static int orangefs_xattr_set_trusted(const struct xattr_handler *handler,
-				     struct dentry *dentry,
-				     const char *name,
-				     const void *buffer,
-				     size_t size,
-				     int flags)
-{
-	return orangefs_inode_setxattr(dentry->d_inode,
-				    XATTR_TRUSTED_PREFIX,
-				    name,
-				    buffer,
-				    size,
-				    flags);
-}
-
-static int orangefs_xattr_get_trusted(const struct xattr_handler *handler,
-				      struct dentry *unused,
-				      struct inode *inode,
-				      const char *name,
-				      void *buffer,
-				      size_t size)
-{
-	return orangefs_inode_getxattr(inode,
-				    XATTR_TRUSTED_PREFIX,
-				    name,
-				    buffer,
-				    size);
-}
-
-static struct xattr_handler orangefs_xattr_trusted_handler = {
-	.prefix = XATTR_TRUSTED_PREFIX,
-	.get = orangefs_xattr_get_trusted,
-	.set = orangefs_xattr_set_trusted,
-};
-
 static struct xattr_handler orangefs_xattr_default_handler = {
 	.prefix = "",  /* match any name => handlers called with full name */
 	.get = orangefs_xattr_get_default,
@@ -521,7 +486,6 @@ static struct xattr_handler orangefs_xattr_default_handler = {
 const struct xattr_handler *orangefs_xattr_handlers[] = {
 	&posix_acl_access_xattr_handler,
 	&posix_acl_default_xattr_handler,
-	&orangefs_xattr_trusted_handler,
 	&orangefs_xattr_default_handler,
 	NULL
 };
-- 
2.5.5

[PATCH 3/3] orangefs: Remove useless xattr prefix arguments

[Andreas Gruenbacher]

Passing the attribute prefix and name to orangefs_inode_*xattr as
separate arguments was unnecessary before, but now that the trusted
xattr handler is gone, the prefix is even always the empty string.
Remove it.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/orangefs/acl.c             |  9 ++---
 fs/orangefs/file.c            |  2 --
 fs/orangefs/orangefs-kernel.h |  2 --
 fs/orangefs/xattr.c           | 81 ++++++++++++++-----------------------------
 4 files changed, 29 insertions(+), 65 deletions(-)

diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
index df24864..28f2195 100644
--- a/fs/orangefs/acl.c
+++ b/fs/orangefs/acl.c
@@ -43,11 +43,8 @@ struct posix_acl *orangefs_get_acl(struct inode *inode, int type)
 		     get_khandle_from_ino(inode),
 		     key,
 		     type);
-	ret = orangefs_inode_getxattr(inode,
-				   "",
-				   key,
-				   value,
-				   ORANGEFS_MAX_XATTR_VALUELEN);
+	ret = orangefs_inode_getxattr(inode, key, value,
+				      ORANGEFS_MAX_XATTR_VALUELEN);
 	/* if the key exists, convert it to an in-memory rep */
 	if (ret > 0) {
 		acl = posix_acl_from_xattr(&init_user_ns, value, ret);
@@ -131,7 +128,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	 * will xlate to a removexattr. However, we don't want removexattr
 	 * complain if attributes does not exist.
 	 */
-	error = orangefs_inode_setxattr(inode, "", name, value, size, 0);
+	error = orangefs_inode_setxattr(inode, name, value, size, 0);
 
 out:
 	kfree(value);
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index 5160a3f..526040e 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -516,7 +516,6 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (cmd == FS_IOC_GETFLAGS) {
 		val = 0;
 		ret = orangefs_inode_getxattr(file_inode(file),
-					      "",
 					      "user.pvfs2.meta_hint",
 					      &val, sizeof(val));
 		if (ret < 0 && ret != -ENODATA)
@@ -549,7 +548,6 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 			     "orangefs_ioctl: FS_IOC_SETFLAGS: %llu\n",
 			     (unsigned long long)val);
 		ret = orangefs_inode_setxattr(file_inode(file),
-					      "",
 					      "user.pvfs2.meta_hint",
 					      &val, sizeof(val), 0);
 	}
diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
index 6503e37..7b542f1 100644
--- a/fs/orangefs/orangefs-kernel.h
+++ b/fs/orangefs/orangefs-kernel.h
@@ -517,13 +517,11 @@ __s32 fsid_of_op(struct orangefs_kernel_op_s *op);
 int orangefs_flush_inode(struct inode *inode);
 
 ssize_t orangefs_inode_getxattr(struct inode *inode,
-			     const char *prefix,
 			     const char *name,
 			     void *buffer,
 			     size_t size);
 
 int orangefs_inode_setxattr(struct inode *inode,
-			 const char *prefix,
 			 const char *name,
 			 const void *value,
 			 size_t size,
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 1d8be91..3fc246b 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -59,8 +59,8 @@ static inline int convert_to_internal_xattr_flags(int setxattr_flags)
  * unless the key does not exist for the file and/or if
  * there were errors in fetching the attribute value.
  */
-ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
-		const char *name, void *buffer, size_t size)
+ssize_t orangefs_inode_getxattr(struct inode *inode, const char *name,
+				void *buffer, size_t size)
 {
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
 	struct orangefs_kernel_op_s *new_op = NULL;
@@ -70,12 +70,12 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
 	int fsgid;
 
 	gossip_debug(GOSSIP_XATTR_DEBUG,
-		     "%s: prefix %s name %s, buffer_size %zd\n",
-		     __func__, prefix, name, size);
+		     "%s: name %s, buffer_size %zd\n",
+		     __func__, name, size);
 
-	if ((strlen(name) + strlen(prefix)) >= ORANGEFS_MAX_XATTR_NAMELEN) {
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
 		gossip_err("Invalid key length (%d)\n",
-			   (int)(strlen(name) + strlen(prefix)));
+			   (int)strlen(name));
 		return -EINVAL;
 	}
 
@@ -97,8 +97,7 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
 		goto out_unlock;
 
 	new_op->upcall.req.getxattr.refn = orangefs_inode->refn;
-	ret = snprintf((char *)new_op->upcall.req.getxattr.key,
-		       ORANGEFS_MAX_XATTR_NAMELEN, "%s%s", prefix, name);
+	strcpy(new_op->upcall.req.getxattr.key, name);
 
 	/*
 	 * NOTE: Although keys are meant to be NULL terminated textual
@@ -163,10 +162,8 @@ out_unlock:
 	return ret;
 }
 
-static int orangefs_inode_removexattr(struct inode *inode,
-			    const char *prefix,
-			    const char *name,
-			    int flags)
+static int orangefs_inode_removexattr(struct inode *inode, const char *name,
+				      int flags)
 {
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
 	struct orangefs_kernel_op_s *new_op = NULL;
@@ -183,12 +180,8 @@ static int orangefs_inode_removexattr(struct inode *inode,
 	 * textual strings, I am going to explicitly pass the
 	 * length just in case we change this later on...
 	 */
-	ret = snprintf((char *)new_op->upcall.req.removexattr.key,
-		       ORANGEFS_MAX_XATTR_NAMELEN,
-		       "%s%s",
-		       (prefix ? prefix : ""),
-		       name);
-	new_op->upcall.req.removexattr.key_sz = ret + 1;
+	strcpy(new_op->upcall.req.removexattr.key, name);
+	new_op->upcall.req.removexattr.key_sz = strlen(name) + 1;
 
 	gossip_debug(GOSSIP_XATTR_DEBUG,
 		     "orangefs_inode_removexattr: key %s, key_sz %d\n",
@@ -223,8 +216,8 @@ out_unlock:
  * Returns a -ve number on error and 0 on success.  Key is text, but value
  * can be binary!
  */
-int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
-		const char *name, const void *value, size_t size, int flags)
+int orangefs_inode_setxattr(struct inode *inode, const char *name,
+			    const void *value, size_t size, int flags)
 {
 	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
 	struct orangefs_kernel_op_s *new_op;
@@ -232,8 +225,8 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
 	int ret = -ENOMEM;
 
 	gossip_debug(GOSSIP_XATTR_DEBUG,
-		     "%s: prefix %s, name %s, buffer_size %zd\n",
-		     __func__, prefix, name, size);
+		     "%s: name %s, buffer_size %zd\n",
+		     __func__, name, size);
 
 	if (size >= ORANGEFS_MAX_XATTR_VALUELEN ||
 	    flags < 0) {
@@ -245,29 +238,19 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
 
 	internal_flag = convert_to_internal_xattr_flags(flags);
 
-	if (prefix) {
-		if (strlen(name) + strlen(prefix) >= ORANGEFS_MAX_XATTR_NAMELEN) {
-			gossip_err
-			    ("orangefs_inode_setxattr: bogus key size (%d)\n",
-			     (int)(strlen(name) + strlen(prefix)));
-			return -EINVAL;
-		}
-	} else {
-		if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
-			gossip_err
-			    ("orangefs_inode_setxattr: bogus key size (%d)\n",
-			     (int)(strlen(name)));
-			return -EINVAL;
-		}
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
+		gossip_err
+		    ("orangefs_inode_setxattr: bogus key size (%d)\n",
+		     (int)(strlen(name)));
+		return -EINVAL;
 	}
 
 	/* This is equivalent to a removexattr */
 	if (size == 0 && value == NULL) {
 		gossip_debug(GOSSIP_XATTR_DEBUG,
-			     "removing xattr (%s%s)\n",
-			     prefix,
+			     "removing xattr (%s)\n",
 			     name);
-		return orangefs_inode_removexattr(inode, prefix, name, flags);
+		return orangefs_inode_removexattr(inode, name, flags);
 	}
 
 	gossip_debug(GOSSIP_XATTR_DEBUG,
@@ -288,11 +271,8 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
 	 * strings, I am going to explicitly pass the length just in
 	 * case we change this later on...
 	 */
-	ret = snprintf((char *)new_op->upcall.req.setxattr.keyval.key,
-		       ORANGEFS_MAX_XATTR_NAMELEN,
-		       "%s%s",
-		       prefix, name);
-	new_op->upcall.req.setxattr.keyval.key_sz = ret + 1;
+	strcpy(new_op->upcall.req.setxattr.keyval.key, name);
+	new_op->upcall.req.setxattr.keyval.key_sz = strlen(name) + 1;
 	memcpy(new_op->upcall.req.setxattr.keyval.val, value, size);
 	new_op->upcall.req.setxattr.keyval.val_sz = size;
 
@@ -454,12 +434,7 @@ static int orangefs_xattr_set_default(const struct xattr_handler *handler,
 				      size_t size,
 				      int flags)
 {
-	return orangefs_inode_setxattr(dentry->d_inode,
-				    "",
-				    name,
-				    buffer,
-				    size,
-				    flags);
+	return orangefs_inode_setxattr(dentry->d_inode, name, buffer, size, flags);
 }
 
 static int orangefs_xattr_get_default(const struct xattr_handler *handler,
@@ -469,11 +444,7 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
 				      void *buffer,
 				      size_t size)
 {
-	return orangefs_inode_getxattr(inode,
-				    "",
-				    name,
-				    buffer,
-				    size);
+	return orangefs_inode_getxattr(inode, name, buffer, size);
 
 }
 
-- 
2.5.5

Re: [PATCH 0/3] Minor orangefs xattr cleanups

[Mike Marshall]

Hi Andreas...

Al Viro worked hard to get us out of the Obfuscated C Contest,
I guess he missed a couple <g>...

Thanks!

-Mike

On Wed, May 25, 2016 at 6:29 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Mike,
>
> here are three patches that clean up small glitches in orangefs xattrs.  Please
> merge.
>
> Thanks,
> Andreas
>
> Andreas Gruenbacher (3):
>   orangefs: Remove useless defines
>   orangefs: Remove redundant "trusted." xattr handler
>   orangefs: Remove useless xattr prefix arguments
>
>  fs/orangefs/acl.c             |  17 +++---
>  fs/orangefs/file.c            |   2 -
>  fs/orangefs/orangefs-kernel.h |  13 -----
>  fs/orangefs/xattr.c           | 124 +++++++++---------------------------------
>  4 files changed, 34 insertions(+), 122 deletions(-)
>
> --
> 2.5.5
>