Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On Wed, 16 Sep 2015 17:30:44 -0400 "bfie...@fieldses.org" wrote: > On Wed, Sep 16, 2015 at 01:18:29PM -0400, Jeff Layton wrote: > > On Mon, 14 Sep 2015 12:10:15 -0400 > > "bfie...@fieldses.org" wrote: > > > > > On Sat, Sep 12, 2015 at 06:24:54AM -0400, Jeff Layton wrote: > > > > I don't think it matters, at least not on x86_64. bools and chars both > > > > require a byte. pahole does show this adding a new hole, but that's > > > > just because this brings the code up to 5 flags and the next field > > > > (fh_pre_size) needs to be aligned. > > > > > > > > I do agree that replacing those other unsigned chars with bools is more > > > > clear however. Maybe we should even replace them all with a single > > > > unsigned int and use bitops to set flags in there. That would be more > > > > space efficient now that we're at 5 flags. > > > > > > Makes sense to me.--b. > > > > I played around with this a little today, and it turns out not to make > > a lot of difference. Here's what pahole says about the existing code > > (once I moved fh_maxsize to snuggle up to fh_handle to plug a hole): > ... > > I used an unsigned long for fh_flags since we might as well. Making it > > smaller just adds a hole in there since the compiler wants to align the > > fh_pre_size. Moving it around doesn't help either as it just moves the > > hole around. Note that this is x86_64. It might look different on a > > 32-bit arch, but I doubt it really matters much in the big scheme of > > things. > > > > Bruce, I'll send out the patches that change this if you like, but I'm > > inclined to just leave this alone since it doesn't seem to have a > > tangible benefit. > > Unless more flags are imminent I guess it's just a question of which is > more readable. Arguably there's some value to making it more obvious > that these are each just a bit. I'll accept your judgement on that > question. > > --b. Sure. Maybe we just switch them to bools. It's the same space utilization but it does make things a little more clear. -- Jeff Layton -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On Wed, Sep 16, 2015 at 01:18:29PM -0400, Jeff Layton wrote: > On Mon, 14 Sep 2015 12:10:15 -0400 > "bfie...@fieldses.org" wrote: > > > On Sat, Sep 12, 2015 at 06:24:54AM -0400, Jeff Layton wrote: > > > I don't think it matters, at least not on x86_64. bools and chars both > > > require a byte. pahole does show this adding a new hole, but that's > > > just because this brings the code up to 5 flags and the next field > > > (fh_pre_size) needs to be aligned. > > > > > > I do agree that replacing those other unsigned chars with bools is more > > > clear however. Maybe we should even replace them all with a single > > > unsigned int and use bitops to set flags in there. That would be more > > > space efficient now that we're at 5 flags. > > > > Makes sense to me.--b. > > I played around with this a little today, and it turns out not to make > a lot of difference. Here's what pahole says about the existing code > (once I moved fh_maxsize to snuggle up to fh_handle to plug a hole): ... > I used an unsigned long for fh_flags since we might as well. Making it > smaller just adds a hole in there since the compiler wants to align the > fh_pre_size. Moving it around doesn't help either as it just moves the > hole around. Note that this is x86_64. It might look different on a > 32-bit arch, but I doubt it really matters much in the big scheme of > things. > > Bruce, I'll send out the patches that change this if you like, but I'm > inclined to just leave this alone since it doesn't seem to have a > tangible benefit. Unless more flags are imminent I guess it's just a question of which is more readable. Arguably there's some value to making it more obvious that these are each just a bit. I'll accept your judgement on that question. --b. -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On Mon, 14 Sep 2015 12:10:15 -0400 "bfie...@fieldses.org" wrote: > On Sat, Sep 12, 2015 at 06:24:54AM -0400, Jeff Layton wrote: > > On Sat, 12 Sep 2015 04:41:33 + > > "Dilger, Andreas" wrote: > > > > > On 2015/09/11, 4:20 AM, "HPDD-discuss on behalf of Jeff Layton" > > > > > > wrote: > > > > > > >With NFSv3 nfsd will always attempt to send along WCC data to the > > > >client. This generally involves saving off the in-core inode information > > > >prior to doing the operation on the given filehandle, and then issuing a > > > >vfs_getattr to it after the op. > > > > > > > >Some filesystems (particularly clustered or networked ones) have an > > > >expensive ->getattr inode operation. Atomicitiy is also often difficult > > > >or impossible to guarantee on such filesystems. For those, we're best > > > >off not trying to provide WCC information to the client at all, and to > > > >simply allow it to poll for that information as needed with a GETATTR > > > >RPC. > > > > > > > >This patch adds a new flags field to struct export_operations, and > > > >defines a new EXPORT_OP_NOWCC flag that filesystems can use to indicate > > > >that nfsd should not attempt to provide WCC info in NFSv3 replies. It > > > >also adds a blurb about the new flags field and flag to the exporting > > > >documentation. > > > > > > > >The server will also now skip collecting this information for NFSv2 as > > > >well, since that info is never used there anyway. > > > > > > > >Note that this patch does not add this flag to any filesystem > > > >export_operations structures. This was originally developed to allow > > > >reexporting nfs via nfsd. That code is not (and may never be) suitable > > > >for merging into mainline. > > > > > > > >Other filesystems may want to consider enabling this flag too. It's hard > > > >to tell however which ones have export operations to enable export via > > > >knfsd and which ones mostly rely on them for open-by-filehandle support, > > > >so I'm leaving that up to the individual maintainers to decide. I am > > > >cc'ing the relevant lists for those filesystems that I think may want to > > > >consider adding this though. > > > > > > > >Cc: hpdd-disc...@lists.01.org > > > >Cc: ceph-devel@vger.kernel.org > > > >Cc: cluster-de...@redhat.com > > > >Cc: fuse-de...@lists.sourceforge.net > > > >Cc: ocfs2-de...@oss.oracle.com > > > >Signed-off-by: Jeff Layton > > > >--- > > > > Documentation/filesystems/nfs/Exporting | 27 +++ > > > > fs/nfsd/nfs3xdr.c | 5 - > > > > fs/nfsd/nfsfh.c | 14 ++ > > > > fs/nfsd/nfsfh.h | 5 - > > > > include/linux/exportfs.h| 2 ++ > > > > 5 files changed, 51 insertions(+), 2 deletions(-) > > > > > > > >diff --git a/Documentation/filesystems/nfs/Exporting > > > >b/Documentation/filesystems/nfs/Exporting > > > >index 520a4becb75c..fa636cde3907 100644 > > > >--- a/Documentation/filesystems/nfs/Exporting > > > >+++ b/Documentation/filesystems/nfs/Exporting > > > >@@ -138,6 +138,11 @@ struct which has the following members: > > > > to find potential names, and matches inode numbers to find the > > > >correct > > > > match. > > > > > > > >+ flags > > > >+Some filesystems may need to be handled differently than others. The > > > >+export_operations struct also includes a flags field that allows the > > > >+filesystem to communicate such information to nfsd. See the Export > > > >+Operations Flags section below for more explanation. > > > > > > > > A filehandle fragment consists of an array of 1 or more 4byte words, > > > > together with a one byte "type". > > > >@@ -147,3 +152,25 @@ generated by encode_fh, in which case it will have > > > >been padded with > > > > nuls. Rather, the encode_fh routine should choose a "type" which > > > > indicates the decode_fh how much of the filehandle is valid, and how > > > > it should be interpreted. > > > >+ > > > >+Export Operations Flags > > > >+--- > > > >+In addition to the operation vector pointers, struct export_operations > > > >also > > > >+contains a "flags" field that allows the filesystem to communicate to > > > >nfsd > > > >+that it may want to do things differently when dealing with it. The > > > >+following flags are defined: > > > >+ > > > >+ EXPORT_OP_NOWCC > > > >+RFC 1813 recommends that servers always send weak cache consistency > > > >+(WCC) data to the client after each operation. The server should > > > >+atomically collect attributes about the inode, do an operation on > > > >it, > > > >+and then collect the attributes afterward. This allows the client to > > > >+skip issuing GETATTRs in some situations but means that the server > > > >+is calling vfs_getattr for almost all RPCs. On some filesystems > > > >+(particularly those that are clustered or networked) this is > > > >expensive > > > >+and atomicity is difficult to
Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On Sat, Sep 12, 2015 at 06:24:54AM -0400, Jeff Layton wrote: > On Sat, 12 Sep 2015 04:41:33 + > "Dilger, Andreas" wrote: > > > On 2015/09/11, 4:20 AM, "HPDD-discuss on behalf of Jeff Layton" > > > > wrote: > > > > >With NFSv3 nfsd will always attempt to send along WCC data to the > > >client. This generally involves saving off the in-core inode information > > >prior to doing the operation on the given filehandle, and then issuing a > > >vfs_getattr to it after the op. > > > > > >Some filesystems (particularly clustered or networked ones) have an > > >expensive ->getattr inode operation. Atomicitiy is also often difficult > > >or impossible to guarantee on such filesystems. For those, we're best > > >off not trying to provide WCC information to the client at all, and to > > >simply allow it to poll for that information as needed with a GETATTR > > >RPC. > > > > > >This patch adds a new flags field to struct export_operations, and > > >defines a new EXPORT_OP_NOWCC flag that filesystems can use to indicate > > >that nfsd should not attempt to provide WCC info in NFSv3 replies. It > > >also adds a blurb about the new flags field and flag to the exporting > > >documentation. > > > > > >The server will also now skip collecting this information for NFSv2 as > > >well, since that info is never used there anyway. > > > > > >Note that this patch does not add this flag to any filesystem > > >export_operations structures. This was originally developed to allow > > >reexporting nfs via nfsd. That code is not (and may never be) suitable > > >for merging into mainline. > > > > > >Other filesystems may want to consider enabling this flag too. It's hard > > >to tell however which ones have export operations to enable export via > > >knfsd and which ones mostly rely on them for open-by-filehandle support, > > >so I'm leaving that up to the individual maintainers to decide. I am > > >cc'ing the relevant lists for those filesystems that I think may want to > > >consider adding this though. > > > > > >Cc: hpdd-disc...@lists.01.org > > >Cc: ceph-devel@vger.kernel.org > > >Cc: cluster-de...@redhat.com > > >Cc: fuse-de...@lists.sourceforge.net > > >Cc: ocfs2-de...@oss.oracle.com > > >Signed-off-by: Jeff Layton > > >--- > > > Documentation/filesystems/nfs/Exporting | 27 +++ > > > fs/nfsd/nfs3xdr.c | 5 - > > > fs/nfsd/nfsfh.c | 14 ++ > > > fs/nfsd/nfsfh.h | 5 - > > > include/linux/exportfs.h| 2 ++ > > > 5 files changed, 51 insertions(+), 2 deletions(-) > > > > > >diff --git a/Documentation/filesystems/nfs/Exporting > > >b/Documentation/filesystems/nfs/Exporting > > >index 520a4becb75c..fa636cde3907 100644 > > >--- a/Documentation/filesystems/nfs/Exporting > > >+++ b/Documentation/filesystems/nfs/Exporting > > >@@ -138,6 +138,11 @@ struct which has the following members: > > > to find potential names, and matches inode numbers to find the > > >correct > > > match. > > > > > >+ flags > > >+Some filesystems may need to be handled differently than others. The > > >+export_operations struct also includes a flags field that allows the > > >+filesystem to communicate such information to nfsd. See the Export > > >+Operations Flags section below for more explanation. > > > > > > A filehandle fragment consists of an array of 1 or more 4byte words, > > > together with a one byte "type". > > >@@ -147,3 +152,25 @@ generated by encode_fh, in which case it will have > > >been padded with > > > nuls. Rather, the encode_fh routine should choose a "type" which > > > indicates the decode_fh how much of the filehandle is valid, and how > > > it should be interpreted. > > >+ > > >+Export Operations Flags > > >+--- > > >+In addition to the operation vector pointers, struct export_operations > > >also > > >+contains a "flags" field that allows the filesystem to communicate to > > >nfsd > > >+that it may want to do things differently when dealing with it. The > > >+following flags are defined: > > >+ > > >+ EXPORT_OP_NOWCC > > >+RFC 1813 recommends that servers always send weak cache consistency > > >+(WCC) data to the client after each operation. The server should > > >+atomically collect attributes about the inode, do an operation on it, > > >+and then collect the attributes afterward. This allows the client to > > >+skip issuing GETATTRs in some situations but means that the server > > >+is calling vfs_getattr for almost all RPCs. On some filesystems > > >+(particularly those that are clustered or networked) this is > > >expensive > > >+and atomicity is difficult to guarantee. This flag indicates to nfsd > > >+that it should skip providing WCC attributes to the client in NFSv3 > > >+replies when doing operations on this filesystem. Consider enabling > > >+this on filesystems that have an expensive ->getattr inode operat
Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On Sat, 12 Sep 2015 04:41:33 + "Dilger, Andreas" wrote: > On 2015/09/11, 4:20 AM, "HPDD-discuss on behalf of Jeff Layton" > > wrote: > > >With NFSv3 nfsd will always attempt to send along WCC data to the > >client. This generally involves saving off the in-core inode information > >prior to doing the operation on the given filehandle, and then issuing a > >vfs_getattr to it after the op. > > > >Some filesystems (particularly clustered or networked ones) have an > >expensive ->getattr inode operation. Atomicitiy is also often difficult > >or impossible to guarantee on such filesystems. For those, we're best > >off not trying to provide WCC information to the client at all, and to > >simply allow it to poll for that information as needed with a GETATTR > >RPC. > > > >This patch adds a new flags field to struct export_operations, and > >defines a new EXPORT_OP_NOWCC flag that filesystems can use to indicate > >that nfsd should not attempt to provide WCC info in NFSv3 replies. It > >also adds a blurb about the new flags field and flag to the exporting > >documentation. > > > >The server will also now skip collecting this information for NFSv2 as > >well, since that info is never used there anyway. > > > >Note that this patch does not add this flag to any filesystem > >export_operations structures. This was originally developed to allow > >reexporting nfs via nfsd. That code is not (and may never be) suitable > >for merging into mainline. > > > >Other filesystems may want to consider enabling this flag too. It's hard > >to tell however which ones have export operations to enable export via > >knfsd and which ones mostly rely on them for open-by-filehandle support, > >so I'm leaving that up to the individual maintainers to decide. I am > >cc'ing the relevant lists for those filesystems that I think may want to > >consider adding this though. > > > >Cc: hpdd-disc...@lists.01.org > >Cc: ceph-devel@vger.kernel.org > >Cc: cluster-de...@redhat.com > >Cc: fuse-de...@lists.sourceforge.net > >Cc: ocfs2-de...@oss.oracle.com > >Signed-off-by: Jeff Layton > >--- > > Documentation/filesystems/nfs/Exporting | 27 +++ > > fs/nfsd/nfs3xdr.c | 5 - > > fs/nfsd/nfsfh.c | 14 ++ > > fs/nfsd/nfsfh.h | 5 - > > include/linux/exportfs.h| 2 ++ > > 5 files changed, 51 insertions(+), 2 deletions(-) > > > >diff --git a/Documentation/filesystems/nfs/Exporting > >b/Documentation/filesystems/nfs/Exporting > >index 520a4becb75c..fa636cde3907 100644 > >--- a/Documentation/filesystems/nfs/Exporting > >+++ b/Documentation/filesystems/nfs/Exporting > >@@ -138,6 +138,11 @@ struct which has the following members: > > to find potential names, and matches inode numbers to find the > >correct > > match. > > > >+ flags > >+Some filesystems may need to be handled differently than others. The > >+export_operations struct also includes a flags field that allows the > >+filesystem to communicate such information to nfsd. See the Export > >+Operations Flags section below for more explanation. > > > > A filehandle fragment consists of an array of 1 or more 4byte words, > > together with a one byte "type". > >@@ -147,3 +152,25 @@ generated by encode_fh, in which case it will have > >been padded with > > nuls. Rather, the encode_fh routine should choose a "type" which > > indicates the decode_fh how much of the filehandle is valid, and how > > it should be interpreted. > >+ > >+Export Operations Flags > >+--- > >+In addition to the operation vector pointers, struct export_operations > >also > >+contains a "flags" field that allows the filesystem to communicate to > >nfsd > >+that it may want to do things differently when dealing with it. The > >+following flags are defined: > >+ > >+ EXPORT_OP_NOWCC > >+RFC 1813 recommends that servers always send weak cache consistency > >+(WCC) data to the client after each operation. The server should > >+atomically collect attributes about the inode, do an operation on it, > >+and then collect the attributes afterward. This allows the client to > >+skip issuing GETATTRs in some situations but means that the server > >+is calling vfs_getattr for almost all RPCs. On some filesystems > >+(particularly those that are clustered or networked) this is > >expensive > >+and atomicity is difficult to guarantee. This flag indicates to nfsd > >+that it should skip providing WCC attributes to the client in NFSv3 > >+replies when doing operations on this filesystem. Consider enabling > >+this on filesystems that have an expensive ->getattr inode operation, > >+or when atomicity between pre and post operation attribute collection > >+is impossible to guarantee. > >diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c > >index 01dcd494f781..c30c8c604e2a 100644 > >--- a/fs/nfsd/nfs3xdr.c > >+++ b/fs/nfsd/n
Re: [HPDD-discuss] [PATCH] nfsd: add a new EXPORT_OP_NOWCC flag to struct export_operations
On 2015/09/11, 4:20 AM, "HPDD-discuss on behalf of Jeff Layton"
wrote:
>With NFSv3 nfsd will always attempt to send along WCC data to the
>client. This generally involves saving off the in-core inode information
>prior to doing the operation on the given filehandle, and then issuing a
>vfs_getattr to it after the op.
>
>Some filesystems (particularly clustered or networked ones) have an
>expensive ->getattr inode operation. Atomicitiy is also often difficult
>or impossible to guarantee on such filesystems. For those, we're best
>off not trying to provide WCC information to the client at all, and to
>simply allow it to poll for that information as needed with a GETATTR
>RPC.
>
>This patch adds a new flags field to struct export_operations, and
>defines a new EXPORT_OP_NOWCC flag that filesystems can use to indicate
>that nfsd should not attempt to provide WCC info in NFSv3 replies. It
>also adds a blurb about the new flags field and flag to the exporting
>documentation.
>
>The server will also now skip collecting this information for NFSv2 as
>well, since that info is never used there anyway.
>
>Note that this patch does not add this flag to any filesystem
>export_operations structures. This was originally developed to allow
>reexporting nfs via nfsd. That code is not (and may never be) suitable
>for merging into mainline.
>
>Other filesystems may want to consider enabling this flag too. It's hard
>to tell however which ones have export operations to enable export via
>knfsd and which ones mostly rely on them for open-by-filehandle support,
>so I'm leaving that up to the individual maintainers to decide. I am
>cc'ing the relevant lists for those filesystems that I think may want to
>consider adding this though.
>
>Cc: hpdd-disc...@lists.01.org
>Cc: ceph-devel@vger.kernel.org
>Cc: cluster-de...@redhat.com
>Cc: fuse-de...@lists.sourceforge.net
>Cc: ocfs2-de...@oss.oracle.com
>Signed-off-by: Jeff Layton
>---
> Documentation/filesystems/nfs/Exporting | 27 +++
> fs/nfsd/nfs3xdr.c | 5 -
> fs/nfsd/nfsfh.c | 14 ++
> fs/nfsd/nfsfh.h | 5 -
> include/linux/exportfs.h| 2 ++
> 5 files changed, 51 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/filesystems/nfs/Exporting
>b/Documentation/filesystems/nfs/Exporting
>index 520a4becb75c..fa636cde3907 100644
>--- a/Documentation/filesystems/nfs/Exporting
>+++ b/Documentation/filesystems/nfs/Exporting
>@@ -138,6 +138,11 @@ struct which has the following members:
> to find potential names, and matches inode numbers to find the
>correct
> match.
>
>+ flags
>+Some filesystems may need to be handled differently than others. The
>+export_operations struct also includes a flags field that allows the
>+filesystem to communicate such information to nfsd. See the Export
>+Operations Flags section below for more explanation.
>
> A filehandle fragment consists of an array of 1 or more 4byte words,
> together with a one byte "type".
>@@ -147,3 +152,25 @@ generated by encode_fh, in which case it will have
>been padded with
> nuls. Rather, the encode_fh routine should choose a "type" which
> indicates the decode_fh how much of the filehandle is valid, and how
> it should be interpreted.
>+
>+Export Operations Flags
>+---
>+In addition to the operation vector pointers, struct export_operations
>also
>+contains a "flags" field that allows the filesystem to communicate to
>nfsd
>+that it may want to do things differently when dealing with it. The
>+following flags are defined:
>+
>+ EXPORT_OP_NOWCC
>+RFC 1813 recommends that servers always send weak cache consistency
>+(WCC) data to the client after each operation. The server should
>+atomically collect attributes about the inode, do an operation on it,
>+and then collect the attributes afterward. This allows the client to
>+skip issuing GETATTRs in some situations but means that the server
>+is calling vfs_getattr for almost all RPCs. On some filesystems
>+(particularly those that are clustered or networked) this is
>expensive
>+and atomicity is difficult to guarantee. This flag indicates to nfsd
>+that it should skip providing WCC attributes to the client in NFSv3
>+replies when doing operations on this filesystem. Consider enabling
>+this on filesystems that have an expensive ->getattr inode operation,
>+or when atomicity between pre and post operation attribute collection
>+is impossible to guarantee.
>diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
>index 01dcd494f781..c30c8c604e2a 100644
>--- a/fs/nfsd/nfs3xdr.c
>+++ b/fs/nfsd/nfs3xdr.c
>@@ -203,7 +203,7 @@ static __be32 *
> encode_post_op_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh
>*fhp)
> {
> struct dentry *dentry = fhp->fh_dentry;
>- if (dentry && d_really_is_positive(dentry)) {
>+ if (!fhp->fh_no_wcc && dentry
