Re: svn commit: r220877 - head/sys/fs/nfsclient
On Wed, 20 Apr 2011, Rick Macklem wrote: Well, its value will be consistent, but not necessarily the "up to date" value set by another thread, if I understood alc@'s recent post. If you haven't yet read it, take a look at his post today on freebsd-hackers@ under the Subject Re: SMP question w.r.t. reading kernel variables I didn't look, but... If I understood his explanation, a thread must lock a mutex that the thread that modified the value has locked/unlocked before it is guaranteed to see that value for the variable instead of some stale memory cached value. (It can be any mutex, but it is probably much easier to use the same one that the modifying thread used during modification instead of finding some other mutex the same thread locked/unlocked after the modification.) I don't see how it can be "any" mutex. "Any" mutex will probably cause some sort of bus lock which will sync the readers and writers at the time of the mutex operation, but not afterwards. I think all that happens is that SMP code executes mutex operations _very_ often. This gives very short race windows which I think hide lots of bugs. Bruce ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r220877 - head/sys/fs/nfsclient
On Wed, 20 Apr 2011, Pawel Jakub Dawidek wrote:
On Wed, Apr 20, 2011 at 08:09:32AM -0400, Rick Macklem wrote:
+ tmp_off = uio->uio_offset + uio->uio_resid;
+ mtx_lock(&nmp->nm_mtx);
+ if (tmp_off > nmp->nm_maxfilesize || tmp_off < uio->uio_offset) {
+ mtx_unlock(&nmp->nm_mtx);
return (EFBIG);
+ }
+ mtx_unlock(&nmp->nm_mtx);
I don't think you need the lock to protect nm_maxfilesize. Can it
change
from under us? My guess is that it is set on mount time and is not
modified afterwards.
I said the same in a private reply which was initially about overflow
errors in the overflow checking, but expanded to also cover errors in
the return values (EFBIG instead of EOF or EOVERFLOW for read()) and
this point.
Good question.
For NFSv3 - it is only modified by the first fsinfo RPC and that normally
happens at mount time, as you guessed above. (This is consistent with
RFC1813, which defines the fsinfo RPC as getting non-volatile information.)
For NFSv4 - it gets it each time VFS_STATFS() happens. I am not sure that
this is correct, but I don't know of anywhere in RFC3530 where it states
that this attribute will not change. In practice, I suspect that servers
seldom, if ever, change it.
So, it is unlikely to change and I'd be comfortable taking the mutex lock
off the check for it, if others are? (As you might be aware, I started a
thread on hackers-freebsd@ where my question was basically "do you need to
mutex lock when you read a global variable". My main concern there was a
case that I'm working on w.r.t. forced dismounts. jhb@ suggested that he
thinks it is good practice to always lock, to play it safe. At least that
was my interpretation?)
This is not that easy, I'm afraid. You need to ask yourself a question
what you are trying to protect from. Here, the mutex only guarantees to
have consistent view of the nm_maxfilesize field. For example if this
field modification wouldn't be atomic you would need the mutex to ensure
that the value is correct.
The modification isn't necessarily atomic since the field is a typedefed
type. The actual type is 64 bits, so the modification happens to be
atomic on 64 bit arches but not on 32 bit arches.
Imagine a situation where it is modifed not by simple 'a = b', but by
something like this:
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = some_32bit_array[0];
nmp->nm_maxfilesize |= some_32bit_array[1] << 32;
mtx_unlock(&nmp->nm_mtx);
To read that properly you need a mutex to ensure you won't read the
value between those two operations.
Modification of 64 bit values on 32 bit arches is essentially the same
as a modification of an array consisting of 2 32 bit elements. However,
for numeric values, it is very unlikely that the modification gives a
fatally invalid in-between value, no matter which order the writes and
reads occur. E.g., for some values, the upper bits might never be changed.
Only modifications like incrementing from 0x to 0x1 may
give fatally different values (this can give a value of 0 from a new value
of 0 in the lower bits and an old value of 0 in the upper bits).
If it is not the case - its modification is atomic and reading it is
atomic then you don't need mutex to read the value as it will always be
consistent. The question is what will happen if it changes after you
read it.
thread0 thread1
--- ---
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = 8192;
mtx_unlock(&nmp->nm_mtx);
mtx_lock(&nmp->nm_mtx);
if (tmp_off > nmp->nm_maxfilesize) {
mtx_unlock(&nmp->nm_mtx);
return (EFBIG);
}
mtx_unlock(&nmp->nm_mtx);
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = 2048;
mtx_unlock(&nmp->nm_mtx);
Now, if tmp_off is 4096 what will happen if you have a race like the
above? Is it critical? Then you need to protect with
this mutex as well.
Locking of the whole transaction is especially impossible for this
field. nm_maxfilesize is from the server (except for local resource
limits which may require reducing it). If it changes after being
initialized, it is probably the server that changed it (since any
change would be foot-shooting so you shouldn't do it locally). There
is no way to prevent the server changing its limit. The copy of the
limit in nm_maxfilesize may have become out of date slightly before
it was written there, or between the write there and the read here,
or after it is checked here, so the server may reject the write for
many slightly different temporal reasons. Any reduction of these
race windows or remote checks that they aren't there or remote locking
to prevent them would be huge pessimizations. So we shouldn't do any
locki
Re: svn commit: r220877 - head/sys/fs/nfsclient
>
> This is not that easy, I'm afraid. You need to ask yourself a question
> what you are trying to protect from. Here, the mutex only guarantees
> to
> have consistent view of the nm_maxfilesize field. For example if this
> field modification wouldn't be atomic you would need the mutex to
> ensure
> that the value is correct.
>
> Imagine a situation where it is modifed not by simple 'a = b', but by
> something like this:
>
> mtx_lock(&nmp->nm_mtx);
> nmp->nm_maxfilesize = some_32bit_array[0];
> nmp->nm_maxfilesize |= some_32bit_array[1] << 32;
> mtx_unlock(&nmp->nm_mtx);
>
> To read that properly you need a mutex to ensure you won't read the
> value between those two operations.
>
> If it is not the case - its modification is atomic and reading it is
> atomic then you don't need mutex to read the value as it will always
> be
> consistent.
Well, its value will be consistent, but not necessarily the "up to date"
value set by another thread, if I understood alc@'s recent post.
If you haven't yet read it, take a look at his post today on freebsd-hackers@
under the Subject
Re: SMP question w.r.t. reading kernel variables
If I understood his explanation, a thread must lock a mutex that the thread
that modified the value has locked/unlocked before it is guaranteed to see
that value for the variable instead of some stale memory cached value.
(It can be any mutex, but it is probably much easier to use the same one
that the modifying thread used during modification instead of finding some
other mutex the same thread locked/unlocked after the modification.)
> The question is what will happen if it changes after you
> read it.
>
> thread0 thread1
> --- ---
>
> mtx_lock(&nmp->nm_mtx);
> nmp->nm_maxfilesize = 8192;
> mtx_unlock(&nmp->nm_mtx);
>
> mtx_lock(&nmp->nm_mtx);
> if (tmp_off > nmp->nm_maxfilesize) {
> mtx_unlock(&nmp->nm_mtx);
> return (EFBIG);
> }
> mtx_unlock(&nmp->nm_mtx);
>
> mtx_lock(&nmp->nm_mtx);
> nmp->nm_maxfilesize = 2048;
> mtx_unlock(&nmp->nm_mtx);
>
>
>
> Now, if tmp_off is 4096 what will happen if you have a race like the
> above? Is it critical? Then you need to protect with
> this mutex as well.
>
Yes, understood. I need to look at the above check w.r.t. nm_maxfilesize
further (such as discussed by bde@). If a server were to change the value
during a read or write, all that should happen is that the server will
return an error for an operation that goes past its specified maxfilesize.
(As you noted, it is unlikely to ever change and, if it did change, I
suspect the server would "grow" it and not "shrink" it.)
The mtx_lock(&nmp->nm_mtx); and mtx_unlock(&nmp->nm_mtx); just ensures
that it reads the "most up to date" value for the variable set by
another thread.
rick
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r220877 - head/sys/fs/nfsclient
On Wed, Apr 20, 2011 at 08:09:32AM -0400, Rick Macklem wrote:
> > > + tmp_off = uio->uio_offset + uio->uio_resid;
> > > + mtx_lock(&nmp->nm_mtx);
> > > + if (tmp_off > nmp->nm_maxfilesize || tmp_off < uio->uio_offset) {
> > > + mtx_unlock(&nmp->nm_mtx);
> > > return (EFBIG);
> > > + }
> > > + mtx_unlock(&nmp->nm_mtx);
> >
> > I don't think you need the lock to protect nm_maxfilesize. Can it
> > change
> > from under us? My guess is that it is set on mount time and is not
> > modified afterwards.
> >
> Good question.
> For NFSv3 - it is only modified by the first fsinfo RPC and that normally
> happens at mount time, as you guessed above. (This is consistent with
> RFC1813, which defines the fsinfo RPC as getting non-volatile
> information.)
> For NFSv4 - it gets it each time VFS_STATFS() happens. I am not sure that
> this is correct, but I don't know of anywhere in RFC3530 where it states
> that this attribute will not change. In practice, I suspect that servers
> seldom, if ever, change it.
>
> So, it is unlikely to change and I'd be comfortable taking the mutex lock
> off the check for it, if others are? (As you might be aware, I started a
> thread on hackers-freebsd@ where my question was basically "do you need to
> mutex lock when you read a global variable". My main concern there was a
> case that I'm working on w.r.t. forced dismounts. jhb@ suggested that he
> thinks it is good practice to always lock, to play it safe. At least that
> was my interpretation?)
This is not that easy, I'm afraid. You need to ask yourself a question
what you are trying to protect from. Here, the mutex only guarantees to
have consistent view of the nm_maxfilesize field. For example if this
field modification wouldn't be atomic you would need the mutex to ensure
that the value is correct.
Imagine a situation where it is modifed not by simple 'a = b', but by
something like this:
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = some_32bit_array[0];
nmp->nm_maxfilesize |= some_32bit_array[1] << 32;
mtx_unlock(&nmp->nm_mtx);
To read that properly you need a mutex to ensure you won't read the
value between those two operations.
If it is not the case - its modification is atomic and reading it is
atomic then you don't need mutex to read the value as it will always be
consistent. The question is what will happen if it changes after you
read it.
thread0 thread1
--- ---
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = 8192;
mtx_unlock(&nmp->nm_mtx);
mtx_lock(&nmp->nm_mtx);
if (tmp_off > nmp->nm_maxfilesize) {
mtx_unlock(&nmp->nm_mtx);
return (EFBIG);
}
mtx_unlock(&nmp->nm_mtx);
mtx_lock(&nmp->nm_mtx);
nmp->nm_maxfilesize = 2048;
mtx_unlock(&nmp->nm_mtx);
Now, if tmp_off is 4096 what will happen if you have a race like the
above? Is it critical? Then you need to protect with
this mutex as well.
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://yomoli.com
pgpgonzRkKE9U.pgp
Description: PGP signature
Re: svn commit: r220877 - head/sys/fs/nfsclient
> > + tmp_off = uio->uio_offset + uio->uio_resid;
> > + mtx_lock(&nmp->nm_mtx);
> > + if (tmp_off > nmp->nm_maxfilesize || tmp_off < uio->uio_offset) {
> > + mtx_unlock(&nmp->nm_mtx);
> > return (EFBIG);
> > + }
> > + mtx_unlock(&nmp->nm_mtx);
>
> I don't think you need the lock to protect nm_maxfilesize. Can it
> change
> from under us? My guess is that it is set on mount time and is not
> modified afterwards.
>
Good question.
For NFSv3 - it is only modified by the first fsinfo RPC and that normally
happens at mount time, as you guessed above. (This is consistent with
RFC1813, which defines the fsinfo RPC as getting non-volatile information.)
For NFSv4 - it gets it each time VFS_STATFS() happens. I am not sure that
this is correct, but I don't know of anywhere in RFC3530 where it states
that this attribute will not change. In practice, I suspect that servers
seldom, if ever, change it.
So, it is unlikely to change and I'd be comfortable taking the mutex lock
off the check for it, if others are? (As you might be aware, I started a
thread on hackers-freebsd@ where my question was basically "do you need to
mutex lock when you read a global variable". My main concern there was a
case that I'm working on w.r.t. forced dismounts. jhb@ suggested that he
thinks it is good practice to always lock, to play it safe. At least that
was my interpretation?)
rick
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r220877 - head/sys/fs/nfsclient
On Wed, Apr 20, 2011 at 01:15:22AM +, Rick Macklem wrote:
> Author: rmacklem
> Date: Wed Apr 20 01:15:22 2011
> New Revision: 220877
> URL: http://svn.freebsd.org/changeset/base/220877
>
> Log:
> Modify the offset + size checks for read and write in the
> experimental NFS client to take care of overflows for the calls
> above the buffer cache layer in a manner similar to r220876.
> Thanks go to dillon at apollo.backplane.com for providing the
> snippet of code that does this.
[...]
> + tmp_off = uio->uio_offset + uio->uio_resid;
> + mtx_lock(&nmp->nm_mtx);
> + if (tmp_off > nmp->nm_maxfilesize || tmp_off < uio->uio_offset) {
> + mtx_unlock(&nmp->nm_mtx);
> return (EFBIG);
> + }
> + mtx_unlock(&nmp->nm_mtx);
I don't think you need the lock to protect nm_maxfilesize. Can it change
from under us? My guess is that it is set on mount time and is not
modified afterwards.
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://yomoli.com
pgppFrhLwoiAo.pgp
Description: PGP signature
