_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, Aug 16, 2007 at 12:10:07PM -0400, Peter Staubach wrote:
> Attached is a new patch which should address the issues raised
> by Bruce.
Thanks!
> I also haven't come to any conclusions regarding the value of
> lease_get_mtime() and whether it should or should not be invoked
> by fill_post_wcc() too. I chose not to change this because I
> thought that it was safer to leave well enough alone. If we
> decide to make a change, it can be done separately.
OK.
Only superficial complaints:
- There were some minor whitespace oddities; running the patch
through scripts/checkpatch.pl may be the quickest way to catch
those.
- This would be better as two, maybe three separate patches;
e.g. moving the lease_get_mtime out of encode_fattr3 could be
done separately first. Ideally we'd do some trivial
transformations like that, followed by one change that
actually changes the inode behavior. That makes the whole
thing trival to review.
I fixed up the first and added the result to
git://linux-nfs.org/~bfields/linux.git for-mm
so it should show up in the next -mm. I'd happily replace it by a more
finely split up version if that was something you could whip up in a few
minutes.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Fri, Aug 17, 2007 at 02:30:31PM -0400, Peter Staubach wrote:
> J. Bruce Fields wrote:
>> - This would be better as two, maybe three separate patches;
>> e.g. moving the lease_get_mtime out of encode_fattr3 could be
>> done separately first. Ideally we'd do some trivial
>> transformations like that, followed by one change that
>> actually changes the inode behavior. That makes the whole
>> thing trival to review.
>
> My test system isn't at a place where I could factor the patch and
> do testing. For the time being, I'd prefer to stick with the single
> patch, since it is tested.
It shouldn't be hard to factor the patch into patches which produce the
identical end result, and are individually trivial enough to be unlikely
to break things partway through.
But the current patch isn't large enough that I'd put my foot down at
this point.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> On Thu, Aug 16, 2007 at 12:10:07PM -0400, Peter Staubach wrote:
>
>> Attached is a new patch which should address the issues raised
>> by Bruce.
>>
>
> Thanks!
>
>
>> I also haven't come to any conclusions regarding the value of
>> lease_get_mtime() and whether it should or should not be invoked
>> by fill_post_wcc() too. I chose not to change this because I
>> thought that it was safer to leave well enough alone. If we
>> decide to make a change, it can be done separately.
>>
>
> OK.
>
> Only superficial complaints:
> - There were some minor whitespace oddities; running the patch
> through scripts/checkpatch.pl may be the quickest way to catch
> those.
>
Ugg. How did those get in there? :-)
Thanx for pointing this out. I will ensure that I run checkpatch
before submitting future patches.
> - This would be better as two, maybe three separate patches;
> e.g. moving the lease_get_mtime out of encode_fattr3 could be
> done separately first. Ideally we'd do some trivial
> transformations like that, followed by one change that
> actually changes the inode behavior. That makes the whole
> thing trival to review.
>
>
My test system isn't at a place where I could factor the patch and
do testing. For the time being, I'd prefer to stick with the single
patch, since it is tested.
> I fixed up the first and added the result to
>
> git://linux-nfs.org/~bfields/linux.git for-mm
>
> so it should show up in the next -mm. I'd happily replace it by a more
> finely split up version if that was something you could whip up in a few
> minutes.
Thanx, Bruce!
ps
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Mon, Aug 06, 2007 at 11:40:58AM -0400, Peter Staubach wrote:
> J. Bruce Fields wrote:
>> On Fri, Aug 03, 2007 at 03:29:10PM -0400, Peter Staubach wrote:
>>> @@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
>>> static __be32 *
>>> encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh
>>> *fhp)
>>> {
>>> - struct inode *inode = fhp->fh_dentry->d_inode;
>>> + if (!fhp->fh_post_saved) {
>>> + *p++ = xdr_zero;
>>> + return p;
>>> + }
>>>
>>
>> The caller, encode_wcc_data(), already did this check.
>
> True. What a twisty maze of small passages, all alike.
>
> The current algorithms don't look quite right to me. I don't
> think that it is valid to return a post_op_attr, which is not
> atomic w.r.t. the operation, even when not returning a pre_op_attr.
> Perhaps a little more cleanup might be good. I will look into
> this.
I *think* the current code is correct, but I agree that it's worth
another check--thanks!
I've also never been really fond of the combined handling of fh_{un}lock
functions that combine the locking and attribute gathering. But maybe
it's just me--I haven't really thought about that code.
>> Dumb question: I assume it's always legal to call ->getattr while
>> holding the i_mutex?
>>
>
> Not so dumb, and I couldn't find an answer to that, either way.
> I couldn't find any reason why it would be illegal, but neither
> did I find explicit reasons for why it would be legal.
>
> Does anyone else know? This gets into the lack of complete
> definition for the virtual file system layer...
There's a useful file Documentation/filesystems/Locking. It has a table
that explains which operations take the i_mutex and which don't. I
assume that a "no" in the table means that function isn't usually called
with the i_mutex, not that it necessarily *must* not be. But I could be
wrong.
That table could use a caption....
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> On Fri, Aug 03, 2007 at 03:29:10PM -0400, Peter Staubach wrote:
>
>> Hi.
>>
>> Attached is a patch to modify the NFS server code to support
>> 64 bit ino's, as appropriate for the system and the NFS
>> protocol version.
>>
>> The gist of the changes is to query the underlying file system
>> for attributes and not just to use the cached attributes in the
>> inode. For this specific purpose, the inode only contains an
>> ino field which unsigned long, which is large enough on 64 bit
>> platforms, but is not large enough on 32 bit platforms.
>>
>
> Thanks!
>
>
>> @@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
>> static __be32 *
>> encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
>> {
>> - struct inode *inode = fhp->fh_dentry->d_inode;
>> + if (!fhp->fh_post_saved) {
>> + *p++ = xdr_zero;
>> + return p;
>> + }
>>
>
> The caller, encode_wcc_data(), already did this check.
>
>
>> /* Attributes to follow */
>> *p++ = xdr_one;
>>
>> - *p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
>> - *p++ = htonl((u32) fhp->fh_post_mode);
>> - *p++ = htonl((u32) fhp->fh_post_nlink);
>> - *p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
>> - *p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
>> - if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
>> - p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
>> - } else {
>> - p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
>> - }
>> - p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
>> - *p++ = fhp->fh_post_rdev[0];
>> - *p++ = fhp->fh_post_rdev[1];
>> - p = encode_fsid(p, fhp);
>> - p = xdr_encode_hyper(p, (u64) inode->i_ino);
>> - p = encode_time3(p, &fhp->fh_post_atime);
>> - p = encode_time3(p, &fhp->fh_post_mtime);
>> - p = encode_time3(p, &fhp->fh_post_ctime);
>> -
>> - return p;
>> + return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
>>
>
> Is there a problem with the lease_get_mtime() call in encode_fattr3()?
> It looks like that could return the current time rather than the time
> that was supposedly atomic with respect to the operation.
>
>
I'm testing a new version of the patch, which addresses this
issue, but then I got to wondering, what is lease_get_mtime()
really for and is it solving a real problem, and if so, is it
solving it in a reasonable fashion?
It appears to me that it is used to detect when an application
on the server has acquired an exclusive lease for a file and
may be modifying it, but without informing the kernel. If it
was informing the kernel, then the mtime would be updated.
NFS doesn't support leases like this, so it would need to be
an application running on the server, either as a standalone
application or as a server for some other service.
Outside of delegations, we don't support anything else like
this, so why is this a special case? Was there a real world
problem that this solved?
And, why does it matter whether the attributes are being
returned via GETATTR, via a post_op_attr, or via a post_op_attr
as part of a wcc_data? The first two cases invoke lease_get_mtime(),
while the third does not. Could this not lead to time jumping
around on a particular file, forwards and backwards?
Thanx...
ps
> Dumb question: I assume it's always legal to call ->getattr while
> holding the i_mutex?
>
> --b.
>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Wed, Aug 08, 2007 at 04:07:37PM -0400, Peter Staubach wrote:
> I'm testing a new version of the patch, which addresses this
> issue, but then I got to wondering, what is lease_get_mtime()
> really for and is it solving a real problem, and if so, is it
> solving it in a reasonable fashion?
>
> It appears to me that it is used to detect when an application
> on the server has acquired an exclusive lease for a file and
> may be modifying it, but without informing the kernel. If it
> was informing the kernel, then the mtime would be updated.
>
> NFS doesn't support leases like this, so it would need to be
> an application running on the server, either as a standalone
> application or as a server for some other service.
Let's say it's Samba, since that's probably what is is.
So Samba is holding a write lease on the file on behalf of one of its
clients. We never see an open from our NFSv2/v3 client--if it doesn't
have data cached, we may never even see a read, just access and
getattr--so we don't know to break that lease.
(Actually, if a write lease, like an NFSv4 write delegation, is meant to
cover file attributes as well as data, then perhaps stat should break
write leases. That sounds painful.)
Anyway, the delay between the time the Samba client makes a change and
the time the NFS client sees it could be unbounded. Whereas with
lease_get_mtime() the client will invalidate its cache and perform a
read, which *does* break the lease (in nfsd_open).
But I agree that this is a strange compromise, and I'd like to
understand better what the semantics for sharing with a Samba client are
supposed to be.
> And, why does it matter whether the attributes are being
> returned via GETATTR, via a post_op_attr, or via a post_op_attr
> as part of a wcc_data? The first two cases invoke lease_get_mtime(),
> while the third does not.
What's the code path in the third case?
> Could this not lead to time jumping around on a particular file,
> forwards and backwards?
Yeah, sounds bad.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> On Wed, Aug 08, 2007 at 04:07:37PM -0400, Peter Staubach wrote:
>
>> I'm testing a new version of the patch, which addresses this
>> issue, but then I got to wondering, what is lease_get_mtime()
>> really for and is it solving a real problem, and if so, is it
>> solving it in a reasonable fashion?
>>
>> It appears to me that it is used to detect when an application
>> on the server has acquired an exclusive lease for a file and
>> may be modifying it, but without informing the kernel. If it
>> was informing the kernel, then the mtime would be updated.
>>
>> NFS doesn't support leases like this, so it would need to be
>> an application running on the server, either as a standalone
>> application or as a server for some other service.
>>
>
> Let's say it's Samba, since that's probably what is is.
>
> So Samba is holding a write lease on the file on behalf of one of its
> clients. We never see an open from our NFSv2/v3 client--if it doesn't
> have data cached, we may never even see a read, just access and
> getattr--so we don't know to break that lease.
>
> (Actually, if a write lease, like an NFSv4 write delegation, is meant to
> cover file attributes as well as data, then perhaps stat should break
> write leases. That sounds painful.)
>
> Anyway, the delay between the time the Samba client makes a change and
> the time the NFS client sees it could be unbounded. Whereas with
> lease_get_mtime() the client will invalidate its cache and perform a
> read, which *does* break the lease (in nfsd_open).
>
> But I agree that this is a strange compromise, and I'd like to
> understand better what the semantics for sharing with a Samba client are
> supposed to be.
>
>
Yes.
>> And, why does it matter whether the attributes are being
>> returned via GETATTR, via a post_op_attr, or via a post_op_attr
>> as part of a wcc_data? The first two cases invoke lease_get_mtime(),
>> while the third does not.
>>
>
> What's the code path in the third case?
>
>
The support in fill_post_wcc() doesn't invoke lease_get_mtime().
Thanx...
ps
>> Could this not lead to time jumping around on a particular file,
>> forwards and backwards?
>>
>
> Yeah, sounds bad.
>
> --b.
>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Wed, Aug 08, 2007 at 04:49:00PM -0400, Peter Staubach wrote:
> J. Bruce Fields wrote:
> > So Samba is holding a write lease on the file on behalf of one of its
> > clients. We never see an open from our NFSv2/v3 client--if it doesn't
> > have data cached, we may never even see a read, just access and
> > getattr--so we don't know to break that lease.
> >
> > (Actually, if a write lease, like an NFSv4 write delegation, is meant to
> > cover file attributes as well as data, then perhaps stat should break
> > write leases. That sounds painful.)
> >
> > Anyway, the delay between the time the Samba client makes a change and
> > the time the NFS client sees it could be unbounded. Whereas with
> > lease_get_mtime() the client will invalidate its cache and perform a
> > read, which *does* break the lease (in nfsd_open).
> >
> > But I agree that this is a strange compromise, and I'd like to
> > understand better what the semantics for sharing with a Samba client are
> > supposed to be.
>
> Yes.
There's a similar problem in the case of, say, a server with both a v3
and a v4 client. If the v4 client has a write delegation, then we break
close-to-open semantics if the v3 client doesn't see change attribute
updates from the v4 client's local modifications. So that's what we
have cb_getattr for. And if you don't want to do cb_getattr calls then
I guess you should be breaking the lease.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Fri, Aug 03, 2007 at 03:29:10PM -0400, Peter Staubach wrote:
> Hi.
>
> Attached is a patch to modify the NFS server code to support
> 64 bit ino's, as appropriate for the system and the NFS
> protocol version.
>
> The gist of the changes is to query the underlying file system
> for attributes and not just to use the cached attributes in the
> inode. For this specific purpose, the inode only contains an
> ino field which unsigned long, which is large enough on 64 bit
> platforms, but is not large enough on 32 bit platforms.
Thanks!
> @@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
> static __be32 *
> encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
> {
> - struct inode *inode = fhp->fh_dentry->d_inode;
> + if (!fhp->fh_post_saved) {
> + *p++ = xdr_zero;
> + return p;
> + }
The caller, encode_wcc_data(), already did this check.
> /* Attributes to follow */
> *p++ = xdr_one;
>
> - *p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
> - *p++ = htonl((u32) fhp->fh_post_mode);
> - *p++ = htonl((u32) fhp->fh_post_nlink);
> - *p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
> - *p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
> - if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
> - p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
> - } else {
> - p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
> - }
> - p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
> - *p++ = fhp->fh_post_rdev[0];
> - *p++ = fhp->fh_post_rdev[1];
> - p = encode_fsid(p, fhp);
> - p = xdr_encode_hyper(p, (u64) inode->i_ino);
> - p = encode_time3(p, &fhp->fh_post_atime);
> - p = encode_time3(p, &fhp->fh_post_mtime);
> - p = encode_time3(p, &fhp->fh_post_ctime);
> -
> - return p;
> + return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
Is there a problem with the lease_get_mtime() call in encode_fattr3()?
It looks like that could return the current time rather than the time
that was supposedly atomic with respect to the operation.
Dumb question: I assume it's always legal to call ->getattr while
holding the i_mutex?
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> On Fri, Aug 03, 2007 at 03:29:10PM -0400, Peter Staubach wrote:
>
>> Hi.
>>
>> Attached is a patch to modify the NFS server code to support
>> 64 bit ino's, as appropriate for the system and the NFS
>> protocol version.
>>
>> The gist of the changes is to query the underlying file system
>> for attributes and not just to use the cached attributes in the
>> inode. For this specific purpose, the inode only contains an
>> ino field which unsigned long, which is large enough on 64 bit
>> platforms, but is not large enough on 32 bit platforms.
>>
>
> Thanks!
>
>
>> @@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
>> static __be32 *
>> encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
>> {
>> - struct inode *inode = fhp->fh_dentry->d_inode;
>> + if (!fhp->fh_post_saved) {
>> + *p++ = xdr_zero;
>> + return p;
>> + }
>>
>
> The caller, encode_wcc_data(), already did this check.
>
>
True. What a twisty maze of small passages, all alike.
The current algorithms don't look quite right to me. I don't
think that it is valid to return a post_op_attr, which is not
atomic w.r.t. the operation, even when not returning a pre_op_attr.
Perhaps a little more cleanup might be good. I will look into
this.
>> /* Attributes to follow */
>> *p++ = xdr_one;
>>
>> - *p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
>> - *p++ = htonl((u32) fhp->fh_post_mode);
>> - *p++ = htonl((u32) fhp->fh_post_nlink);
>> - *p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
>> - *p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
>> - if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
>> - p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
>> - } else {
>> - p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
>> - }
>> - p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
>> - *p++ = fhp->fh_post_rdev[0];
>> - *p++ = fhp->fh_post_rdev[1];
>> - p = encode_fsid(p, fhp);
>> - p = xdr_encode_hyper(p, (u64) inode->i_ino);
>> - p = encode_time3(p, &fhp->fh_post_atime);
>> - p = encode_time3(p, &fhp->fh_post_mtime);
>> - p = encode_time3(p, &fhp->fh_post_ctime);
>> -
>> - return p;
>> + return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
>>
>
> Is there a problem with the lease_get_mtime() call in encode_fattr3()?
> It looks like that could return the current time rather than the time
> that was supposedly atomic with respect to the operation.
>
>
Sorry, I glossed over that one. Let me address that. Good catch.
> Dumb question: I assume it's always legal to call ->getattr while
> holding the i_mutex?
>
Not so dumb, and I couldn't find an answer to that, either way.
I couldn't find any reason why it would be illegal, but neither
did I find explicit reasons for why it would be legal.
Does anyone else know? This gets into the lack of complete
definition for the virtual file system layer...
Thanx for the review, Bruce.
Thanx...
ps
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs