2011-08-29 18:51:34

by Chuck Lever III

[permalink] [raw]
Subject: [PATCH] NFSD: Add a cache for fs_locations information

From: Trond Myklebust <[email protected]>

Signed-off-by: Trond Myklebust <[email protected]>
[ cel: since this is server-side, use nfsd4_ prefix instead of nfs4_ prefix. ]
Signed-off-by: Chuck Lever <[email protected]>
---

fs/nfsd/nfsd.h | 7 +++++++
fs/nfsd/vfs.c | 15 +++++++++++++++
2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 7ecfa24..d314812 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -335,6 +335,13 @@ static inline u32 nfsd_suppattrs2(u32 minorversion)
#define NFSD_SUPPATTR_EXCLCREAT_WORD2 \
NFSD_WRITEABLE_ATTRS_WORD2

+extern int nfsd4_is_junction(struct dentry *dentry);
+#else
+static inline int nfsd4_is_junction(struct dentry *dentry)
+{
+ return 0;
+}
+
#endif /* CONFIG_NFSD_V4 */

#endif /* LINUX_NFSD_NFSD_H */
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fd0acca..5dac667 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -168,6 +168,8 @@ int nfsd_mountpoint(struct dentry *dentry, struct svc_export *exp)
{
if (d_mountpoint(dentry))
return 1;
+ if (nfsd4_is_junction(dentry))
+ return 1;
if (!(exp->ex_flags & NFSEXP_V4ROOT))
return 0;
return dentry->d_inode != NULL;
@@ -592,6 +594,19 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry, struct nfs4_ac
return error;
}

+#define NFSD_XATTR_JUNCTION_PREFIX XATTR_TRUSTED_PREFIX "junction."
+#define NFSD_XATTR_JUNCTION_TYPE NFSD_XATTR_JUNCTION_PREFIX "type"
+int nfsd4_is_junction(struct dentry *dentry)
+{
+ ssize_t ret;
+
+ if (dentry->d_inode != NULL) {
+ ret = vfs_getxattr(dentry, NFSD_XATTR_JUNCTION_TYPE, NULL, 0);
+ if (ret > 0)
+ return 1;
+ }
+ return 0;
+}
#endif /* defined(CONFIG_NFSD_V4) */

#ifdef CONFIG_NFSD_V3



2011-08-30 20:31:05

by Boaz Harrosh

[permalink] [raw]
Subject: Re: [PATCH] NFSD: Add a cache for fs_locations information

On 08/30/2011 01:00 PM, Chuck Lever wrote:
<>
>>
>> Won't the above check be rather expensive? You'll need to do a
>> getxattr call on almost every path component of every lookup,
>> right?
>>
>> I may be misremembering your talk from connectathon, but I thought
>> you were planning to use a well-known mode for junctions that would
>> cut down on the number of unnecessary getxattrs...
>
> Yes, that's the plan. To reduce overhead, the S_ISVTX bit must be
> set before NFSD does the expensive xattr test.

from: stat(2) - Linux man page

The 'sticky' bit (S_ISVTX) on a directory means that a file in that
directory can be renamed or deleted only by the owner of the file,
by the owner of the directory, and by a privileged process.

Please explain how does it work? Once the junction is followed and
mounted then the mode-bits get changed to the destination directory's
mode bits? So the Server's junction mode-bits are never exposed, except
in a local-fs file access on the server?

Thanks
Boaz

> However, I don't
> think this kind of filtering was ever implemented. I got the patch
> from here:
>
> http://git.linux-nfs.org/?p=trondmy/nfs-2.6.git;a=shortlog;h=refs/heads/fedfs-for-2.6.34
>
> and that doesn't seem to have it either. I can implement something
> and repost these.
>

2011-08-30 20:34:14

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH] NFSD: Add a cache for fs_locations information

On Tue, 2011-08-30 at 13:24 -0700, Boaz Harrosh wrote:
> On 08/30/2011 01:00 PM, Chuck Lever wrote:
> <>
> >>
> >> Won't the above check be rather expensive? You'll need to do a
> >> getxattr call on almost every path component of every lookup,
> >> right?
> >>
> >> I may be misremembering your talk from connectathon, but I thought
> >> you were planning to use a well-known mode for junctions that would
> >> cut down on the number of unnecessary getxattrs...
> >
> > Yes, that's the plan. To reduce overhead, the S_ISVTX bit must be
> > set before NFSD does the expensive xattr test.

...and mode bits otherwise set to 0 so nobody can access the mounted-on
directory.

> from: stat(2) - Linux man page
>
> The 'sticky' bit (S_ISVTX) on a directory means that a file in that
> directory can be renamed or deleted only by the owner of the file,
> by the owner of the directory, and by a privileged process.
>
> Please explain how does it work? Once the junction is followed and
> mounted then the mode-bits get changed to the destination directory's
> mode bits? So the Server's junction mode-bits are never exposed, except
> in a local-fs file access on the server?

Yes.



--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com


2011-08-30 21:03:59

by Boaz Harrosh

[permalink] [raw]
Subject: Re: [PATCH] NFSD: Add a cache for fs_locations information

On 08/30/2011 01:34 PM, Trond Myklebust wrote:
> On Tue, 2011-08-30 at 13:24 -0700, Boaz Harrosh wrote:
>> On 08/30/2011 01:00 PM, Chuck Lever wrote:
>> <>
>>>>
>>>> Won't the above check be rather expensive? You'll need to do a
>>>> getxattr call on almost every path component of every lookup,
>>>> right?
>>>>
>>>> I may be misremembering your talk from connectathon, but I thought
>>>> you were planning to use a well-known mode for junctions that would
>>>> cut down on the number of unnecessary getxattrs...
>>>
>>> Yes, that's the plan. To reduce overhead, the S_ISVTX bit must be
>>> set before NFSD does the expensive xattr test.
>
> ...and mode bits otherwise set to 0 so nobody can access the mounted-on
> directory.
>
>> from: stat(2) - Linux man page
>>
>> The 'sticky' bit (S_ISVTX) on a directory means that a file in that
>> directory can be renamed or deleted only by the owner of the file,
>> by the owner of the directory, and by a privileged process.
>>
>> Please explain how does it work? Once the junction is followed and
>> mounted then the mode-bits get changed to the destination directory's
>> mode bits? So the Server's junction mode-bits are never exposed, except
>> in a local-fs file access on the server?
>
> Yes.
>
Nice trick. Thanks

2011-08-30 20:01:09

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH] NFSD: Add a cache for fs_locations information


On Aug 30, 2011, at 3:18 PM, Jeff Layton wrote:

> On Mon, 29 Aug 2011 14:51:15 -0400
> Chuck Lever <[email protected]> wrote:
>
>> From: Trond Myklebust <[email protected]>
>>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> [ cel: since this is server-side, use nfsd4_ prefix instead of nfs4_ prefix. ]
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> fs/nfsd/nfsd.h | 7 +++++++
>> fs/nfsd/vfs.c | 15 +++++++++++++++
>> 2 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index 7ecfa24..d314812 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -335,6 +335,13 @@ static inline u32 nfsd_suppattrs2(u32 minorversion)
>> #define NFSD_SUPPATTR_EXCLCREAT_WORD2 \
>> NFSD_WRITEABLE_ATTRS_WORD2
>>
>> +extern int nfsd4_is_junction(struct dentry *dentry);
>> +#else
>> +static inline int nfsd4_is_junction(struct dentry *dentry)
>> +{
>> + return 0;
>> +}
>> +
>> #endif /* CONFIG_NFSD_V4 */
>>
>> #endif /* LINUX_NFSD_NFSD_H */
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index fd0acca..5dac667 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -168,6 +168,8 @@ int nfsd_mountpoint(struct dentry *dentry, struct svc_export *exp)
>> {
>> if (d_mountpoint(dentry))
>> return 1;
>> + if (nfsd4_is_junction(dentry))
>> + return 1;
>> if (!(exp->ex_flags & NFSEXP_V4ROOT))
>> return 0;
>> return dentry->d_inode != NULL;
>> @@ -592,6 +594,19 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry, struct nfs4_ac
>> return error;
>> }
>>
>> +#define NFSD_XATTR_JUNCTION_PREFIX XATTR_TRUSTED_PREFIX "junction."
>> +#define NFSD_XATTR_JUNCTION_TYPE NFSD_XATTR_JUNCTION_PREFIX "type"
>> +int nfsd4_is_junction(struct dentry *dentry)
>> +{
>> + ssize_t ret;
>> +
>> + if (dentry->d_inode != NULL) {
>> + ret = vfs_getxattr(dentry, NFSD_XATTR_JUNCTION_TYPE, NULL, 0);
>> + if (ret > 0)
>> + return 1;
>> + }
>> + return 0;
>> +}
>
> Won't the above check be rather expensive? You'll need to do a
> getxattr call on almost every path component of every lookup, right?
>
> I may be misremembering your talk from connectathon, but I thought you
> were planning to use a well-known mode for junctions that would cut
> down on the number of unnecessary getxattrs...

Yes, that's the plan. To reduce overhead, the S_ISVTX bit must be set before NFSD does the expensive xattr test. However, I don't think this kind of filtering was ever implemented. I got the patch from here:

http://git.linux-nfs.org/?p=trondmy/nfs-2.6.git;a=shortlog;h=refs/heads/fedfs-for-2.6.34

and that doesn't seem to have it either. I can implement something and repost these.

>> #endif /* defined(CONFIG_NFSD_V4) */
>>
>> #ifdef CONFIG_NFSD_V3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> --
> Jeff Layton <[email protected]>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com





2011-08-30 19:18:49

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH] NFSD: Add a cache for fs_locations information

On Mon, 29 Aug 2011 14:51:15 -0400
Chuck Lever <[email protected]> wrote:

> From: Trond Myklebust <[email protected]>
>
> Signed-off-by: Trond Myklebust <[email protected]>
> [ cel: since this is server-side, use nfsd4_ prefix instead of nfs4_ prefix. ]
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> fs/nfsd/nfsd.h | 7 +++++++
> fs/nfsd/vfs.c | 15 +++++++++++++++
> 2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 7ecfa24..d314812 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -335,6 +335,13 @@ static inline u32 nfsd_suppattrs2(u32 minorversion)
> #define NFSD_SUPPATTR_EXCLCREAT_WORD2 \
> NFSD_WRITEABLE_ATTRS_WORD2
>
> +extern int nfsd4_is_junction(struct dentry *dentry);
> +#else
> +static inline int nfsd4_is_junction(struct dentry *dentry)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_NFSD_V4 */
>
> #endif /* LINUX_NFSD_NFSD_H */
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index fd0acca..5dac667 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -168,6 +168,8 @@ int nfsd_mountpoint(struct dentry *dentry, struct svc_export *exp)
> {
> if (d_mountpoint(dentry))
> return 1;
> + if (nfsd4_is_junction(dentry))
> + return 1;
> if (!(exp->ex_flags & NFSEXP_V4ROOT))
> return 0;
> return dentry->d_inode != NULL;
> @@ -592,6 +594,19 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry, struct nfs4_ac
> return error;
> }
>
> +#define NFSD_XATTR_JUNCTION_PREFIX XATTR_TRUSTED_PREFIX "junction."
> +#define NFSD_XATTR_JUNCTION_TYPE NFSD_XATTR_JUNCTION_PREFIX "type"
> +int nfsd4_is_junction(struct dentry *dentry)
> +{
> + ssize_t ret;
> +
> + if (dentry->d_inode != NULL) {
> + ret = vfs_getxattr(dentry, NFSD_XATTR_JUNCTION_TYPE, NULL, 0);
> + if (ret > 0)
> + return 1;
> + }
> + return 0;
> +}

Won't the above check be rather expensive? You'll need to do a
getxattr call on almost every path component of every lookup, right?

I may be misremembering your talk from connectathon, but I thought you
were planning to use a well-known mode for junctions that would cut
down on the number of unnecessary getxattrs...

> #endif /* defined(CONFIG_NFSD_V4) */
>
> #ifdef CONFIG_NFSD_V3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


--
Jeff Layton <[email protected]>