| Message ID | 1415448664-25815-3-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Nov 08, 2014 at 01:11:04PM +0100, Christoph Hellwig wrote: > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/nfsd/nfs4xdr.c | 16 ++++++++++++++++ > fs/nfsd/nfsd.h | 2 +- > include/linux/nfs4.h | 9 +++++++++ > 3 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index eeea7a9..3205b55 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -1730,6 +1730,15 @@ static __be32 *encode_change(__be32 *p, struct kstat *stat, struct inode *inode) > return p; > } > > +static __be32 *encode_change_attr_type(__be32 *p, struct inode *inode) > +{ > + if (IS_I_VERSION(inode)) > + *p++ = cpu_to_be32(NFS4_CHANGE_TYPE_IS_VERSION_COUNTER); Shouldn't that be NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR? The draft says that e.g. "If the client sees NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, it has the ability to predict what the resulting change attribute value should be after a COMPOUND containing a SETATTR, WRITE, or CREATE." Admittedly, I'm not completely sure what that means. (Is a SETATTR of multiple attributes a single atomic change? Can we predict the change attribute on a newly created file, or only on the parent directory?) I also don't know where the filesystems do the i_version increment (can we guarantee it happens once per nfs WRITE?). --b. > + else > + *p++ = cpu_to_be32(NFS4_CHANGE_TYPE_IS_TIME_METADATA); > + return p; > +} > + > static __be32 *encode_cinfo(__be32 *p, struct nfsd4_change_info *c) > { > *p++ = cpu_to_be32(c->atomic); > @@ -2535,6 +2544,13 @@ out_acl: > *p++ = cpu_to_be32(NFSD_SUPPATTR_EXCLCREAT_WORD2); > } > > + if (bmval2 & FATTR4_WORD2_CHANGE_ATTR_TYPE) { > + p = xdr_reserve_space(xdr, 4); > + if (!p) > + goto out_resource; > + p = encode_change_attr_type(p, dentry->d_inode); > + } > + > attrlen = htonl(xdr->buf->len - attrlen_offset - 4); > write_bytes_to_xdr_buf(xdr->buf, attrlen_offset, &attrlen, 4); > status = nfs_ok; > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h > index 43b6a36..59a734f 100644 > --- a/fs/nfsd/nfsd.h > +++ b/fs/nfsd/nfsd.h > @@ -342,7 +342,7 @@ void nfsd_lockd_shutdown(void); > > #define NFSD4_2_SUPPORTED_ATTRS_WORD2 \ > (NFSD4_1_SUPPORTED_ATTRS_WORD2 | \ > - NFSD4_2_SECURITY_ATTRS) > + FATTR4_WORD2_CHANGE_ATTR_TYPE | NFSD4_2_SECURITY_ATTRS) > > static inline u32 nfsd_suppattrs0(u32 minorversion) > { > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > index 356acc2..85ccd06 100644 > --- a/include/linux/nfs4.h > +++ b/include/linux/nfs4.h > @@ -413,6 +413,7 @@ enum lock_type4 { > #define FATTR4_WORD1_FS_LAYOUT_TYPES (1UL << 30) > #define FATTR4_WORD2_LAYOUT_BLKSIZE (1UL << 1) > #define FATTR4_WORD2_MDSTHRESHOLD (1UL << 4) > +#define FATTR4_WORD2_CHANGE_ATTR_TYPE (1UL << 15) > #define FATTR4_WORD2_SECURITY_LABEL (1UL << 16) > > /* MDS threshold bitmap bits */ > @@ -558,4 +559,12 @@ enum data_content4 { > NFS4_CONTENT_HOLE = 1, > }; > > +enum change_attr_type4 { > + NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR = 0, > + NFS4_CHANGE_TYPE_IS_VERSION_COUNTER = 1, > + NFS4_CHANGE_TYPE_IS_VERSION_COUNTER_NOPNFS = 2, > + NFS4_CHANGE_TYPE_IS_TIME_METADATA = 3, > + NFS4_CHANGE_TYPE_IS_UNDEFINED = 4 > +}; > + > #endif > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 10, 2014 at 12:54:24PM -0500, J. Bruce Fields wrote: > Shouldn't that be NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR? > > The draft says that e.g. "If the client sees > NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, it has the ability to predict what > the resulting change attribute value should be after a COMPOUND > containing a SETATTR, WRITE, or CREATE." > > Admittedly, I'm not completely sure what that means. (Is a SETATTR of > multiple attributes a single atomic change? Can we predict the change > attribute on a newly created file, or only on the parent directory?) I > also don't know where the filesystems do the i_version increment (can we > guarantee it happens once per nfs WRITE?). Actually the server may increment it many times for a single WRITE, for XFS it is incremented for each dirty transaction, which could happen many times during a single write: (1) c/mtime update (2) suid/sgid bit removal (3) block allocation (could be multiple transactions) So I guess we really should move to NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR instead. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 11, 2014 at 5:27 AM, Christoph Hellwig <hch@lst.de> wrote: > On Mon, Nov 10, 2014 at 12:54:24PM -0500, J. Bruce Fields wrote: >> Shouldn't that be NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR? >> >> The draft says that e.g. "If the client sees >> NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, it has the ability to predict what >> the resulting change attribute value should be after a COMPOUND >> containing a SETATTR, WRITE, or CREATE." >> >> Admittedly, I'm not completely sure what that means. (Is a SETATTR of >> multiple attributes a single atomic change? Can we predict the change >> attribute on a newly created file, or only on the parent directory?) I >> also don't know where the filesystems do the i_version increment (can we >> guarantee it happens once per nfs WRITE?). > > Actually the server may increment it many times for a single WRITE, > for XFS it is incremented for each dirty transaction, which could > happen many times during a single write: > > (1) c/mtime update > (2) suid/sgid bit removal > (3) block allocation (could be multiple transactions) > > > So I guess we really should move to NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR > instead. Agreed. Just out of curiosity, though, why does XFS update i_version on every block allocation? For NFSv4 compatibility, it really should suffice for it to just do the update once in the above case. Did Lustre have more stringent requirements?
On Tue, Nov 11, 2014 at 07:36:48AM -0500, Trond Myklebust wrote: > Just out of curiosity, though, why does XFS update i_version on every > block allocation? For NFSv4 compatibility, it really should suffice > for it to just do the update once in the above case. Did Lustre have > more stringent requirements? It's literatlly updated everytime inode metadata changes. Dave implemented this, so I don't know the real rationale behind it. I don't think anyone sane cares about Lustre ever, certainly not for XFS. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 11, 2014 at 11:27:35AM +0100, Christoph Hellwig wrote: > On Mon, Nov 10, 2014 at 12:54:24PM -0500, J. Bruce Fields wrote: > > Shouldn't that be NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR? > > > > The draft says that e.g. "If the client sees > > NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, it has the ability to predict what > > the resulting change attribute value should be after a COMPOUND > > containing a SETATTR, WRITE, or CREATE." > > > > Admittedly, I'm not completely sure what that means. (Is a SETATTR of > > multiple attributes a single atomic change? Can we predict the change > > attribute on a newly created file, or only on the parent directory?) I > > also don't know where the filesystems do the i_version increment (can we > > guarantee it happens once per nfs WRITE?). > > Actually the server may increment it many times for a single WRITE, > for XFS it is incremented for each dirty transaction, which could > happen many times during a single write: > > (1) c/mtime update > (2) suid/sgid bit removal > (3) block allocation (could be multiple transactions) > > > So I guess we really should move to NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR > instead. I'm applying your patch for 3.19 with that one-line change. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > To clarify what Christoph wrote, XFS updates i_version is updated > once per transaction that modifies the inode. So if a VFS level > operation results in multiple transactions then each transaction > will but the version. > > It was implemented that way because nobody could tell me what the > actual granularity requirement for change detection was. Hence what > I implemented was "be able to detect any persistent change that is > made" to cover all bases. Honestly the XFS implementation seems most sensible, and easiest to verify for me. I don't really understand the rationale behind the fairly convoluted NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics, and I doubt you could actually implemet them on any Unix-like semantics. Trond, given that the language in the standard is from you: 1) how do you expect to use NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics in the client 2) what server do you have in mind that could actually implement them? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: >> To clarify what Christoph wrote, XFS updates i_version is updated >> once per transaction that modifies the inode. So if a VFS level >> operation results in multiple transactions then each transaction >> will but the version. >> >> It was implemented that way because nobody could tell me what the >> actual granularity requirement for change detection was. Hence what >> I implemented was "be able to detect any persistent change that is >> made" to cover all bases. > > Honestly the XFS implementation seems most sensible, and easiest to > verify for me. I don't really understand the rationale behind the > fairly convoluted NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics, and > I doubt you could actually implemet them on any Unix-like semantics. > > Trond, given that the language in the standard is from you: > > 1) how do you expect to use NFS4_CHANGE_TYPE_IS_VERSION_COUNTER > semantics in the client Basically, I'd like to use it the same way that AFS does. I want to be able to issue an RPC call which does the equivalent of a single system call (e.g. mkdir(), write(), link(), unlink(), etc) and be able to predict what the effect should be on the change attribute (1 increment on the parent directory for a successful mkdir(), 1 increment on the file for a successful write(), ...) so that I can detect if someone else has been modifying the file/directory/symlink while I wasn't looking and hence know when I need to invalidate my cached metadata+data for that object. > 2) what server do you have in mind that could actually implement them? As I said, AFS has this kind of semantics, and the AFS client on Linux uses them to do this kind of 3rd party change detection.
On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: > On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > >> To clarify what Christoph wrote, XFS updates i_version is updated > >> once per transaction that modifies the inode. So if a VFS level > >> operation results in multiple transactions then each transaction > >> will but the version. > >> > >> It was implemented that way because nobody could tell me what the > >> actual granularity requirement for change detection was. Hence what > >> I implemented was "be able to detect any persistent change that is > >> made" to cover all bases. > > > > Honestly the XFS implementation seems most sensible, and easiest to > > verify for me. I don't really understand the rationale behind the > > fairly convoluted NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics, and > > I doubt you could actually implemet them on any Unix-like semantics. > > > > Trond, given that the language in the standard is from you: > > > > 1) how do you expect to use NFS4_CHANGE_TYPE_IS_VERSION_COUNTER > > semantics in the client > > Basically, I'd like to use it the same way that AFS does. I want to be > able to issue an RPC call which does the equivalent of a single system > call (e.g. mkdir(), write(), link(), unlink(), etc) and be able to > predict what the effect should be on the change attribute (1 increment > on the parent directory for a successful mkdir(), 1 increment on the > file for a successful write(), ...) That's not the way the change version counter is implemented in the VFS or any filesystem. It's a low level change primitive, not something that is only updated on a syscall granularity. I just can't see how a change counter at the syscall level can be made to work reliably. NFS clients are now being told about server block maps, so any extent map modification done by the underlying filesystem needs to bump the change count so if the client is caching the block map it can be invalidated. And with functionality like delayed allocation modifications the client needs to know aout can happen at any time and so change count modification can not be limited only to syscall activity. > so that I can detect if someone > else has been modifying the file/directory/symlink while I wasn't > looking and hence know when I need to invalidate my cached > metadata+data for that object. The only way to use the change count sanely from the client is as a "check-and-execute" cookie on the server. If the change count sent by the client is unchanged at the server then the server can execute the operation. It can then return the new cookie to the client for the next operation. But we can't even do that sanely on Linux because the check-and-execute operation needs to be atomic and hence requires the filesystem to do it deep inside their transaction subsystems once they've taken the locks it needs to ensure the change count is stable. Cheers, Dave.
On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: >> >> To clarify what Christoph wrote, XFS updates i_version is updated >> >> once per transaction that modifies the inode. So if a VFS level >> >> operation results in multiple transactions then each transaction >> >> will but the version. >> >> >> >> It was implemented that way because nobody could tell me what the >> >> actual granularity requirement for change detection was. Hence what >> >> I implemented was "be able to detect any persistent change that is >> >> made" to cover all bases. >> > >> > Honestly the XFS implementation seems most sensible, and easiest to >> > verify for me. I don't really understand the rationale behind the >> > fairly convoluted NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics, and >> > I doubt you could actually implemet them on any Unix-like semantics. >> > >> > Trond, given that the language in the standard is from you: >> > >> > 1) how do you expect to use NFS4_CHANGE_TYPE_IS_VERSION_COUNTER >> > semantics in the client >> >> Basically, I'd like to use it the same way that AFS does. I want to be >> able to issue an RPC call which does the equivalent of a single system >> call (e.g. mkdir(), write(), link(), unlink(), etc) and be able to >> predict what the effect should be on the change attribute (1 increment >> on the parent directory for a successful mkdir(), 1 increment on the >> file for a successful write(), ...) > > That's not the way the change version counter is implemented in the > VFS or any filesystem. It's a low level change primitive, not > something that is only updated on a syscall granularity. > > I just can't see how a change counter at the syscall level can be > made to work reliably. NFS clients are now being told about server > block maps, so any extent map modification done by the underlying > filesystem needs to bump the change count so if the client is > caching the block map it can be invalidated. And with functionality > like delayed allocation modifications the client needs to know aout > can happen at any time and so change count modification can not be > limited only to syscall activity. I didn't say it needs to be implemented in the VFS. Just that it needs to be implemented in a way that makes sense if you are doing the equivalent of a system call. Delayed allocations are a filesystem implementation detail that do not change the application visible data or metadata contents of the file; there should be no reason to have them reflected in something like the change attribute. As for pNFS blocks, I agree that the spec there is a little iffy, but the intention was, I believe, that the whole LAYOUTGET->LAYOUTCOMMIT should be considered to be a single filesystem transaction. However the iffiness there is the main reason why I made a distinction between pnfs vs. non-pnfs when describing the change attribute. >> so that I can detect if someone >> else has been modifying the file/directory/symlink while I wasn't >> looking and hence know when I need to invalidate my cached >> metadata+data for that object. > > The only way to use the change count sanely from the client is as a > "check-and-execute" cookie on the server. If the change count sent > by the client is unchanged at the server then the server can execute > the operation. It can then return the new cookie to the client for > the next operation. But we can't even do that sanely on Linux > because the check-and-execute operation needs to be atomic and hence > requires the filesystem to do it deep inside their transaction > subsystems once they've taken the locks it needs to ensure the > change count is stable. Applications are required to interact with the filesystem through a well-defined API. Application visible data and metadata changes can be (and are mostly) well defined w.r.t. that API. Where be the dragons?
On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: > On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: > >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > >> >> To clarify what Christoph wrote, XFS updates i_version is updated > >> >> once per transaction that modifies the inode. So if a VFS level > >> >> operation results in multiple transactions then each transaction > >> >> will but the version. > >> >> > >> >> It was implemented that way because nobody could tell me what the > >> >> actual granularity requirement for change detection was. Hence what > >> >> I implemented was "be able to detect any persistent change that is > >> >> made" to cover all bases. > >> > > >> > Honestly the XFS implementation seems most sensible, and easiest to > >> > verify for me. I don't really understand the rationale behind the > >> > fairly convoluted NFS4_CHANGE_TYPE_IS_VERSION_COUNTER semantics, and > >> > I doubt you could actually implemet them on any Unix-like semantics. > >> > > >> > Trond, given that the language in the standard is from you: > >> > > >> > 1) how do you expect to use NFS4_CHANGE_TYPE_IS_VERSION_COUNTER > >> > semantics in the client > >> > >> Basically, I'd like to use it the same way that AFS does. I want to be > >> able to issue an RPC call which does the equivalent of a single system > >> call (e.g. mkdir(), write(), link(), unlink(), etc) and be able to > >> predict what the effect should be on the change attribute (1 increment > >> on the parent directory for a successful mkdir(), 1 increment on the > >> file for a successful write(), ...) > > > > That's not the way the change version counter is implemented in the > > VFS or any filesystem. It's a low level change primitive, not > > something that is only updated on a syscall granularity. > > > > I just can't see how a change counter at the syscall level can be > > made to work reliably. NFS clients are now being told about server > > block maps, so any extent map modification done by the underlying > > filesystem needs to bump the change count so if the client is > > caching the block map it can be invalidated. And with functionality > > like delayed allocation modifications the client needs to know aout > > can happen at any time and so change count modification can not be > > limited only to syscall activity. > > I didn't say it needs to be implemented in the VFS. Just that it needs > to be implemented in a way that makes sense if you are doing the > equivalent of a system call. Your requirements indicate that the functionality can only be implemented in the VFS - filesystems themselves have no idea what system call operations are, and often they get multiple entries from the VFS for one system call. i.e. "one increment per syscall" is not a problem individual filesystems can solve. > Delayed allocations are a filesystem > implementation detail that do not change the application visible data > or metadata contents of the file; there should be no reason to have > them reflected in something like the change attribute. Delayed allocation changes inode metadata in user visible ways because extent maps are user visible metadata..... > As for pNFS blocks, I agree that the spec there is a little iffy, but > the intention was, I believe, that the whole LAYOUTGET->LAYOUTCOMMIT > should be considered to be a single filesystem transaction. However > the iffiness there is the main reason why I made a distinction between > pnfs vs. non-pnfs when describing the change attribute. Forget pNFS, all the new stuff for exposing sparse files to the client (e.g. FIEMAP, SEEK_DATA/HOLE) as well as preallocation mean that filesystem allocation of any kind causes application visible metadata changes on a file. > >> so that I can detect if someone > >> else has been modifying the file/directory/symlink while I wasn't > >> looking and hence know when I need to invalidate my cached > >> metadata+data for that object. > > > > The only way to use the change count sanely from the client is as a > > "check-and-execute" cookie on the server. If the change count sent > > by the client is unchanged at the server then the server can execute > > the operation. It can then return the new cookie to the client for > > the next operation. But we can't even do that sanely on Linux > > because the check-and-execute operation needs to be atomic and hence > > requires the filesystem to do it deep inside their transaction > > subsystems once they've taken the locks it needs to ensure the > > change count is stable. > > Applications are required to interact with the filesystem through a > well-defined API. Application visible data and metadata changes can be > (and are mostly) well defined w.r.t. that API. Where be the dragons? That API isn't as well defined as you think. Every filesystem has it's own ioctls that allow all sorts of interesting things to be done. What we define as "application visible" appears to be different because our problem scope is different. That's where the dragons lie, and why you're going to need explicit NFS behaviour to be implemented through the VFS if you want a specific set of well defined behaviours. Otherwise you are going to get whatever each individual filesystem considers a change in that counter. And given that, clients simply can't make assumptions about how server side counters change w.r.t. modifications being made because servers (and potentially even exports on a server) are going to differ in behaviour. The only sane way to detect a change has occurred is for the change counter to be returned as a post-op attribute on a file operation and for that value to be used as a pre-condition for the next operation to be performed that is sent to the server.... Cheers, Dave.
On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: > On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: > >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > >> >> To clarify what Christoph wrote, XFS updates i_version is updated > >> >> once per transaction that modifies the inode. So if a VFS level > >> >> operation results in multiple transactions then each transaction > >> >> will but the version. > >> >> > >> >> It was implemented that way because nobody could tell me what the > >> >> actual granularity requirement for change detection was. Hence what > >> >> I implemented was "be able to detect any persistent change that is > >> >> made" to cover all bases. FWIW, ext4 takes the same approach. See Ted's post today: http://www.spinics.net/lists/linux-ext4/msg46194.html "The inode_inc_iversion() in mark4_ext4_iloc_dirty() is probably not necessary, since we already should be incrementing i_version whenever ctime and mtime gets updated. The inode_inc_iversion() there is more of a "belt and suspenders" safety thing, on the theory that the extra bump in i_version won't hurt anything." Cheers, Dave.
On Thu, Nov 13, 2014 at 6:54 PM, Dave Chinner <david@fromorbit.com> wrote: > On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: >> On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: >> > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: >> >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: >> >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: >> >> >> To clarify what Christoph wrote, XFS updates i_version is updated >> >> >> once per transaction that modifies the inode. So if a VFS level >> >> >> operation results in multiple transactions then each transaction >> >> >> will but the version. >> >> >> >> >> >> It was implemented that way because nobody could tell me what the >> >> >> actual granularity requirement for change detection was. Hence what >> >> >> I implemented was "be able to detect any persistent change that is >> >> >> made" to cover all bases. > > FWIW, ext4 takes the same approach. See Ted's post today: > > http://www.spinics.net/lists/linux-ext4/msg46194.html > > "The inode_inc_iversion() in mark4_ext4_iloc_dirty() is probably not > necessary, since we already should be incrementing i_version whenever > ctime and mtime gets updated. The inode_inc_iversion() there is more > of a "belt and suspenders" safety thing, on the theory that the extra > bump in i_version won't hurt anything." > It will hurt if it causes all the NFS clients to blow their caches unnecessarily. Who asked for this?
On Thu, Nov 13, 2014 at 07:43:33PM -0500, Trond Myklebust wrote: > On Thu, Nov 13, 2014 at 6:54 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: > >> On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: > >> > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: > >> >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > >> >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > >> >> >> To clarify what Christoph wrote, XFS updates i_version is updated > >> >> >> once per transaction that modifies the inode. So if a VFS level > >> >> >> operation results in multiple transactions then each transaction > >> >> >> will but the version. > >> >> >> > >> >> >> It was implemented that way because nobody could tell me what the > >> >> >> actual granularity requirement for change detection was. Hence what > >> >> >> I implemented was "be able to detect any persistent change that is > >> >> >> made" to cover all bases. > > > > FWIW, ext4 takes the same approach. See Ted's post today: > > > > http://www.spinics.net/lists/linux-ext4/msg46194.html > > > > "The inode_inc_iversion() in mark4_ext4_iloc_dirty() is probably not > > necessary, since we already should be incrementing i_version whenever > > ctime and mtime gets updated. The inode_inc_iversion() there is more > > of a "belt and suspenders" safety thing, on the theory that the extra > > bump in i_version won't hurt anything." > > > > It will hurt if it causes all the NFS clients to blow their caches > unnecessarily. Not my problem. We've just implemented what we were asked to implement. > Who asked for this? The only discussion where actual specifications were enumerated was during a thread about using i_version in the integrity measurement code (IMA subsystem). The NFSv4 requirements for the change counter were expressed here: https://lkml.org/lkml/2012/1/5/408 Don't blame us for implementing the vague "changes every time" requirements in a way that results in no chance of a persistent change to either data or metadata being missed by the filesystem. Cheers, Dave.
On Thu, Nov 13, 2014 at 11:35 PM, Dave Chinner <david@fromorbit.com> wrote: > On Thu, Nov 13, 2014 at 07:43:33PM -0500, Trond Myklebust wrote: >> On Thu, Nov 13, 2014 at 6:54 PM, Dave Chinner <david@fromorbit.com> wrote: >> > On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: >> >> On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: >> >> > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: >> >> >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: >> >> >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: >> >> >> >> To clarify what Christoph wrote, XFS updates i_version is updated >> >> >> >> once per transaction that modifies the inode. So if a VFS level >> >> >> >> operation results in multiple transactions then each transaction >> >> >> >> will but the version. >> >> >> >> >> >> >> >> It was implemented that way because nobody could tell me what the >> >> >> >> actual granularity requirement for change detection was. Hence what >> >> >> >> I implemented was "be able to detect any persistent change that is >> >> >> >> made" to cover all bases. >> > >> > FWIW, ext4 takes the same approach. See Ted's post today: >> > >> > http://www.spinics.net/lists/linux-ext4/msg46194.html >> > >> > "The inode_inc_iversion() in mark4_ext4_iloc_dirty() is probably not >> > necessary, since we already should be incrementing i_version whenever >> > ctime and mtime gets updated. The inode_inc_iversion() there is more >> > of a "belt and suspenders" safety thing, on the theory that the extra >> > bump in i_version won't hurt anything." >> > >> >> It will hurt if it causes all the NFS clients to blow their caches >> unnecessarily. > > Not my problem. We've just implemented what we were asked to > implement. > >> Who asked for this? > > The only discussion where actual specifications were enumerated was > during a thread about using i_version in the integrity measurement > code (IMA subsystem). The NFSv4 requirements for the change counter > were expressed here: > > https://lkml.org/lkml/2012/1/5/408 > > Don't blame us for implementing the vague "changes every time" > requirements in a way that results in no chance of a persistent > change to either data or metadata being missed by the filesystem. I'm not blaming anyone. I'm stating that I'm not aware of anybody who needs to trace fiemap changes via the change attribute, and so I'm asking where that requirement came from? The NFSv4 requirements are specific if you take them within the context of NFS, which is all about clients not having to care about whether the data is stored on SSD block devices or deduped stone tablets. Fiemap, being block specific, is irrelevant to that environment. So do the IMA folks use i_version and, if so, how are they using it?
On Fri, Nov 14, 2014 at 09:22:14AM -0500, Trond Myklebust wrote: > On Thu, Nov 13, 2014 at 11:35 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Nov 13, 2014 at 07:43:33PM -0500, Trond Myklebust wrote: > >> On Thu, Nov 13, 2014 at 6:54 PM, Dave Chinner <david@fromorbit.com> wrote: > >> > On Thu, Nov 13, 2014 at 08:02:43AM -0500, Trond Myklebust wrote: > >> >> On Wed, Nov 12, 2014 at 7:28 PM, Dave Chinner <david@fromorbit.com> wrote: > >> >> > On Wed, Nov 12, 2014 at 09:26:16AM -0500, Trond Myklebust wrote: > >> >> >> On Wed, Nov 12, 2014 at 5:24 AM, Christoph Hellwig <hch@lst.de> wrote: > >> >> >> > On Wed, Nov 12, 2014 at 09:27:10AM +1100, Dave Chinner wrote: > >> >> >> >> To clarify what Christoph wrote, XFS updates i_version is updated > >> >> >> >> once per transaction that modifies the inode. So if a VFS level > >> >> >> >> operation results in multiple transactions then each transaction > >> >> >> >> will but the version. > >> >> >> >> > >> >> >> >> It was implemented that way because nobody could tell me what the > >> >> >> >> actual granularity requirement for change detection was. Hence what > >> >> >> >> I implemented was "be able to detect any persistent change that is > >> >> >> >> made" to cover all bases. > >> > > >> > FWIW, ext4 takes the same approach. See Ted's post today: > >> > > >> > http://www.spinics.net/lists/linux-ext4/msg46194.html > >> > > >> > "The inode_inc_iversion() in mark4_ext4_iloc_dirty() is probably not > >> > necessary, since we already should be incrementing i_version whenever > >> > ctime and mtime gets updated. The inode_inc_iversion() there is more > >> > of a "belt and suspenders" safety thing, on the theory that the extra > >> > bump in i_version won't hurt anything." > >> > > >> > >> It will hurt if it causes all the NFS clients to blow their caches > >> unnecessarily. > > > > Not my problem. We've just implemented what we were asked to > > implement. > > > >> Who asked for this? > > > > The only discussion where actual specifications were enumerated was > > during a thread about using i_version in the integrity measurement > > code (IMA subsystem). The NFSv4 requirements for the change counter > > were expressed here: > > > > https://lkml.org/lkml/2012/1/5/408 > > > > Don't blame us for implementing the vague "changes every time" > > requirements in a way that results in no chance of a persistent > > change to either data or metadata being missed by the filesystem. > > I'm not blaming anyone. I'm stating that I'm not aware of anybody who > needs to trace fiemap changes via the change attribute, and so I'm > asking where that requirement came from? It was seriously being considered - it appeared as a potential in NFSv4 draft specs for handling sparse file reads. Indeed, this draft directly mentions reading block maps from XFS and using it on the client side: http://tools.ietf.org/html/draft-hildebrand-nfsv4-read-sparse-00 "XFS supports the XFS_IOC_GETBMAP extended attribute, which returns the allocation information for a file. Clients can then use this information to only read allocated data blocks" Now, I know that was from 2010, and the eventual 2014 NFSv4.2 RFC doesn't have this in it, but go back 3-4 years ago when we were trying to work out to how make an on-disk version counter work sanely for all the different things we'd been hearing about were going to be necessary for NFSv4.... Cheers, Dave.
On Tue, Nov 11, 2014 at 04:42:06PM -0500, J. Bruce Fields wrote: > On Tue, Nov 11, 2014 at 11:27:35AM +0100, Christoph Hellwig wrote: > > On Mon, Nov 10, 2014 at 12:54:24PM -0500, J. Bruce Fields wrote: > > > Shouldn't that be NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR? > > > > > > The draft says that e.g. "If the client sees > > > NFS4_CHANGE_TYPE_IS_VERSION_COUNTER, it has the ability to predict what > > > the resulting change attribute value should be after a COMPOUND > > > containing a SETATTR, WRITE, or CREATE." > > > > > > Admittedly, I'm not completely sure what that means. (Is a SETATTR of > > > multiple attributes a single atomic change? Can we predict the change > > > attribute on a newly created file, or only on the parent directory?) I > > > also don't know where the filesystems do the i_version increment (can we > > > guarantee it happens once per nfs WRITE?). > > > > Actually the server may increment it many times for a single WRITE, > > for XFS it is incremented for each dirty transaction, which could > > happen many times during a single write: > > > > (1) c/mtime update > > (2) suid/sgid bit removal > > (3) block allocation (could be multiple transactions) > > > > > > So I guess we really should move to NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR > > instead. > > I'm applying your patch for 3.19 with that one-line change. Actually, I'm having second thoughts given that there still seems to be some argument about whether the change_attr_type thing is completely right. Maybe that only affects values other than CHANGE_TYPE_IS_MONOTIC_INCR, but is there any urgency if we don't have a client user for it yet? --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 01, 2014 at 02:50:21PM -0500, J. Bruce Fields wrote: > Actually, I'm having second thoughts given that there still seems to be > some argument about whether the change_attr_type thing is completely > right. Maybe that only affects values other than > CHANGE_TYPE_IS_MONOTIC_INCR, but is there any urgency if we don't have a > client user for it yet? I don't see a real urgency, just wanted to make sure we have reference implementations for more of 4.2 available. I'd defintively be curious about how the client could make use of these. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index eeea7a9..3205b55 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1730,6 +1730,15 @@ static __be32 *encode_change(__be32 *p, struct kstat *stat, struct inode *inode) return p; } +static __be32 *encode_change_attr_type(__be32 *p, struct inode *inode) +{ + if (IS_I_VERSION(inode)) + *p++ = cpu_to_be32(NFS4_CHANGE_TYPE_IS_VERSION_COUNTER); + else + *p++ = cpu_to_be32(NFS4_CHANGE_TYPE_IS_TIME_METADATA); + return p; +} + static __be32 *encode_cinfo(__be32 *p, struct nfsd4_change_info *c) { *p++ = cpu_to_be32(c->atomic); @@ -2535,6 +2544,13 @@ out_acl: *p++ = cpu_to_be32(NFSD_SUPPATTR_EXCLCREAT_WORD2); } + if (bmval2 & FATTR4_WORD2_CHANGE_ATTR_TYPE) { + p = xdr_reserve_space(xdr, 4); + if (!p) + goto out_resource; + p = encode_change_attr_type(p, dentry->d_inode); + } + attrlen = htonl(xdr->buf->len - attrlen_offset - 4); write_bytes_to_xdr_buf(xdr->buf, attrlen_offset, &attrlen, 4); status = nfs_ok; diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h index 43b6a36..59a734f 100644 --- a/fs/nfsd/nfsd.h +++ b/fs/nfsd/nfsd.h @@ -342,7 +342,7 @@ void nfsd_lockd_shutdown(void); #define NFSD4_2_SUPPORTED_ATTRS_WORD2 \ (NFSD4_1_SUPPORTED_ATTRS_WORD2 | \ - NFSD4_2_SECURITY_ATTRS) + FATTR4_WORD2_CHANGE_ATTR_TYPE | NFSD4_2_SECURITY_ATTRS) static inline u32 nfsd_suppattrs0(u32 minorversion) { diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h index 356acc2..85ccd06 100644 --- a/include/linux/nfs4.h +++ b/include/linux/nfs4.h @@ -413,6 +413,7 @@ enum lock_type4 { #define FATTR4_WORD1_FS_LAYOUT_TYPES (1UL << 30) #define FATTR4_WORD2_LAYOUT_BLKSIZE (1UL << 1) #define FATTR4_WORD2_MDSTHRESHOLD (1UL << 4) +#define FATTR4_WORD2_CHANGE_ATTR_TYPE (1UL << 15) #define FATTR4_WORD2_SECURITY_LABEL (1UL << 16) /* MDS threshold bitmap bits */ @@ -558,4 +559,12 @@ enum data_content4 { NFS4_CONTENT_HOLE = 1, }; +enum change_attr_type4 { + NFS4_CHANGE_TYPE_IS_MONOTONIC_INCR = 0, + NFS4_CHANGE_TYPE_IS_VERSION_COUNTER = 1, + NFS4_CHANGE_TYPE_IS_VERSION_COUNTER_NOPNFS = 2, + NFS4_CHANGE_TYPE_IS_TIME_METADATA = 3, + NFS4_CHANGE_TYPE_IS_UNDEFINED = 4 +}; + #endif
Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/nfsd/nfs4xdr.c | 16 ++++++++++++++++ fs/nfsd/nfsd.h | 2 +- include/linux/nfs4.h | 9 +++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)