Fwd: CIFS data coherency problem

Fwd: CIFS data coherency problem

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

---------- Forwarded message ----------
From: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: 2010/9/8
Subject: CIFS data coherency problem
To: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Копия: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org


Hello!

I faced with a problem of the wrong cifs cache behavior while adapting
CIFS VFS client for working with the application which uses file
system as a mechanism for storing data and organizing paralell access
from several client. If we look at CIFS code, we can see that it uses
kernel cache mechanism all the time (do_sync_read, do_sync_write, etc)
and delegate all the checking for validating data to cifs_revalidate
call.

cifs_revalidate call uses QueryInfo protocol command for checking
mtime and file size. I noticed that the server doesn't update mtime
every time we writng to the server - that's why we can't use it.

On another hand CIFS spec says that the client can't use cache for if
it doesn't have an oplock - if we don't follow the spec, we can faced
with other problems.

Even more: if we use a Windows server and the mandatory locking style,
now we can read from locking by other clients range (if we have this
data in cache) - it's not right.

As the solution, I suggest to follow the spec in every it's part: to
do cache write/read if we have Exclusive oplock, to do cache read if
we have Oplock Level II and as for other cases - use direct operations
with the server.

I attached the test (cache_problem.py) that shows the problem.

What do you think about it? I have the code that do read/write
according to the spec but I want to discuss this question before
posting the patch because I think it's rather important

--
Best regards,
Pavel Shilovsky.



-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: cache_problem.py --]
[-- Type: application/octet-stream, Size: 717 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
write(f, ''.join('a' for _ in range(4096)))
close(f)

f1 = open('test1/_test4321_', O_RDWR)
f2 = open('test2/_test4321_', O_RDWR)

write(f1, 'x')
print 'x is written through f1'
print '%c is read from f2' % read(f2, 1)

write(f1, 'y')
print 'y is written through f1'
print '%c is read from f2' % read(f2, 1)

write(f1, 'z')
print 'z is written through f1'
print '%c is read from f2' % read(f2, 1)

close(f1)
close(f2)

Re: CIFS data coherency problem

[Jeff Layton]

On Wed, 8 Sep 2010 10:49:13 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Hello!
> 
> I faced with a problem of the wrong cifs cache behavior while adapting
> CIFS VFS client for working with the application which uses file
> system as a mechanism for storing data and organizing paralell access
> from several client. If we look at CIFS code, we can see that it uses
> kernel cache mechanism all the time (do_sync_read, do_sync_write, etc)
> and delegate all the checking for validating data to cifs_revalidate
> call.
> 
> cifs_revalidate call uses QueryInfo protocol command for checking
> mtime and file size. I noticed that the server doesn't update mtime
> every time we writng to the server - that's why we can't use it.
> 

What sort of server were you working with here?

> On another hand CIFS spec says that the client can't use cache for if
> it doesn't have an oplock - if we don't follow the spec, we can faced
> with other problems.
> 
> Even more: if we use a Windows server and the mandatory locking style,
> now we can read from locking by other clients range (if we have this
> data in cache) - it's not right.
> 
> As the solution, I suggest to follow the spec in every it's part: to
> do cache write/read if we have Exclusive oplock, to do cache read if
> we have Oplock Level II and as for other cases - use direct operations
> with the server.
> 
> I attached the test (cache_problem.py) that shows the problem.
> 
> What do you think about it? I have the code that do read/write
> according to the spec but I want to discuss this question before
> posting the patch because I think it's rather important
> 

At the very least, we should consider a "strict" caching model like you
describe as an option. If it has a large performance impact then we may
want to allow people to use the existing caching model as well. OTOH,
maintaining multiple caching models may be too cumbersome to support.

If you're saying however that the spec says this, then it would
also be helpful to point out the place in the spec that outlines this
so we can go back and read over that part.

There's no need to hesitate in sending the patches however. Doing so
gives us a starting point for discussing the change. If you're not sure
about them, just declare them an "RFC".

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Jeff Layton]

On Wed, 8 Sep 2010 06:32:24 -0400
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> On Wed, 8 Sep 2010 10:49:13 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Hello!
> > 
> > I faced with a problem of the wrong cifs cache behavior while adapting
> > CIFS VFS client for working with the application which uses file
> > system as a mechanism for storing data and organizing paralell access
> > from several client. If we look at CIFS code, we can see that it uses
> > kernel cache mechanism all the time (do_sync_read, do_sync_write, etc)
> > and delegate all the checking for validating data to cifs_revalidate
> > call.
> > 
> > cifs_revalidate call uses QueryInfo protocol command for checking
> > mtime and file size. I noticed that the server doesn't update mtime
> > every time we writng to the server - that's why we can't use it.
> > 
> 
> What sort of server were you working with here?
> 
> > On another hand CIFS spec says that the client can't use cache for if
> > it doesn't have an oplock - if we don't follow the spec, we can faced
> > with other problems.
> > 
> > Even more: if we use a Windows server and the mandatory locking style,
> > now we can read from locking by other clients range (if we have this
> > data in cache) - it's not right.
> > 
> > As the solution, I suggest to follow the spec in every it's part: to
> > do cache write/read if we have Exclusive oplock, to do cache read if
> > we have Oplock Level II and as for other cases - use direct operations
> > with the server.
> > 
> > I attached the test (cache_problem.py) that shows the problem.
> > 
> > What do you think about it? I have the code that do read/write
> > according to the spec but I want to discuss this question before
> > posting the patch because I think it's rather important
> > 
> 
> At the very least, we should consider a "strict" caching model like you
> describe as an option. If it has a large performance impact then we may
> want to allow people to use the existing caching model as well. OTOH,
> maintaining multiple caching models may be too cumbersome to support.
> 
> If you're saying however that the spec says this, then it would
> also be helpful to point out the place in the spec that outlines this
> so we can go back and read over that part.
> 
> There's no need to hesitate in sending the patches however. Doing so
> gives us a starting point for discussing the change. If you're not sure
> about them, just declare them an "RFC".
> 

Also, while you're considering the semantics here...

How should mmap work if you do not have an oplock?

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/8 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Wed, 8 Sep 2010 10:49:13 +0400
>
> What sort of server were you working with here?

Samba 3.4, Samba 4, Windows XP.

>
> At the very least, we should consider a "strict" caching model like you
> describe as an option. If it has a large performance impact then we may
> want to allow people to use the existing caching model as well. OTOH,
> maintaining multiple caching models may be too cumbersome to support.

Yes, It brings complexity. As for performance: how can we talk about
performance if we have invalid data? In this case when we can't use
any serious application for business on CIFS file system (because of
wrong data coherency) why do we need such a performance? I think we at
first should keep all the data up to date and then think about
performance and other not so important things.

>
> If you're saying however that the spec says this, then it would
> also be helpful to point out the place in the spec that outlines this
> so we can go back and read over that part.

3.2.4.18 from [MS-CIFS].pdf states when a client should use a cache
for reading, writing or locking. You can also look at
http://go.microsoft.com/fwlink/?LinkID=140636 for more information
about oplock semantic. In every case I didn't noticed any information
for using cache if we don't have an oplock. If you have such an
information it'll be very interesting to see it.

>
> There's no need to hesitate in sending the patches however. Doing so
> gives us a starting point for discussing the change. If you're not sure
> about them, just declare them an "RFC".

Ok, I will prepare the patch and send it soon.

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/8 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> Also, while you're considering the semantics here...
>
> How should mmap work if you do not have an oplock?

The good question - I am not sure, but I think it won't break things
if we leave it the same. If I am not right, correct me, please.

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Steve French]

On Thu, Sep 9, 2010 at 1:18 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2010/9/8 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> On Wed, 8 Sep 2010 10:49:13 +0400
>>
>> What sort of server were you working with here?
>
> Samba 3.4, Samba 4, Windows XP.
>
>>
>> At the very least, we should consider a "strict" caching model like you
>> describe as an option. If it has a large performance impact then we may
>> want to allow people to use the existing caching model as well. OTOH,
>> maintaining multiple caching models may be too cumbersome to support.
>
> Yes, It brings complexity. As for performance: how can we talk about
> performance if we have invalid data? In this case when we can't use
> any serious application for business on CIFS file system (because of
> wrong data coherency) why do we need such a performance? I think we at
> first should keep all the data up to date and then think about
> performance and other not so important things.
>
>>
>> If you're saying however that the spec says this, then it would
>> also be helpful to point out the place in the spec that outlines this
>> so we can go back and read over that part.
>
> 3.2.4.18 from [MS-CIFS].pdf states when a client should use a cache
> for reading, writing or locking. You can also look at
> http://go.microsoft.com/fwlink/?LinkID=140636 for more information
> about oplock semantic. In every case I didn't noticed any information
> for using cache if we don't have an oplock. If you have such an
> information it'll be very interesting to see it.

Note that metadata caching is common for most network file systems
(not unique to cifs or nfs) - and is usually done on a timer.

NFSv3 data caching is unsafe - no guarantees data makes it to server
until sync or close.  Generally if a file has not changed between
close and open - seems reasonable to keep the cached copy.   If we
have two opens of the same file from different clients we won't be
caching anyway.

In SMB2 we will be doing batch oplock.


-- 
Thanks,

Steve

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/9 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Note that metadata caching is common for most network file systems
> (not unique to cifs or nfs) - and is usually done on a timer.
>
> NFSv3 data caching is unsafe - no guarantees data makes it to server
> until sync or close.  Generally if a file has not changed between
> close and open - seems reasonable to keep the cached copy.   If we

As I mentioned in the first email, CIFS servers doesn't update mtime
of a file every time and that brings complexity. So we can't be sure
that we have actual data of a file in a cache.

> have two opens of the same file from different clients we won't be
> caching anyway.

But now CIFS kernel client does it - it uses cache. As for SMB2 - it's the same.

>
> In SMB2 we will be doing batch oplock.
>

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Steve French]

On Thu, Sep 9, 2010 at 1:34 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2010/9/9 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> Note that metadata caching is common for most network file systems
>> (not unique to cifs or nfs) - and is usually done on a timer.
>>
>> NFSv3 data caching is unsafe - no guarantees data makes it to server
>> until sync or close.  Generally if a file has not changed between
>> close and open - seems reasonable to keep the cached copy.   If we
>
> As I mentioned in the first email, CIFS servers doesn't update mtime
> of a file every time and that brings complexity. So we can't be sure
> that we have actual data of a file in a cache.

Surely it is a serious bug if a server doesn't update the mtime by the
time the handle they used is closed.   If a client 1 does open/write/close,
then client 2 does open/write/close, client 1 reopening the file should
see the updated mtime.   If client 2 had not closed the file yet - it
is not clear whether its write and mtime update will be processed
first - but we shouldn't be using cached data in that case - client 1 should
do an invalidate_mapping when it can't get an oplock (we do that already
in seek and mmap via revalidate e.g.).  In any case writes won't be cached
in that case - and the simplest change may be to invalidate the inode
cached pages in this reopen path - when the mtime/size matches but we
failed to get a read oplock.   Allowing us to turn off the 1 second timeout
on metadata/data caching is already possible.

>> have two opens of the same file from different clients we won't be
>> caching anyway.

With no oplock we won't be caching writes - we do cache reads
in some cases (the intent originally was to do this for about 1 second)
but as you note we can cache longer


-- 
Thanks,

Steve

Re: CIFS data coherency problem

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 3266 bytes --]

2010/9/10 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Surely it is a serious bug if a server doesn't update the mtime by the
> time the handle they used is closed.   If a client 1 does open/write/close,
> then client 2 does open/write/close, client 1 reopening the file should
> see the updated mtime.   If client 2 had not closed the file yet - it
> is not clear whether its write and mtime update will be processed
> first - but we shouldn't be using cached data in that case - client 1 should
> do an invalidate_mapping when it can't get an oplock (we do that already
> in seek and mmap via revalidate e.g.).  In any case writes won't be cached
> in that case - and the simplest change may be to invalidate the inode
> cached pages in this reopen path - when the mtime/size matches but we

I mean the situation then we have this file opened by another file handle. E.g.:

1) client1 opens file as f1.
2) client1 writes 'a' into f1 from the beginning.
3) client2 opens file as f2.
4) client2 writes 'x' into f2 to the beginning,
5) client1 opens file as f3.
6) client1 reads from f3 and get 'a'!!! But it must be 'x'!

I attached this test 'mtime_problem.py' and the capture
'mtime_problem.pcap'. On the capture you can see that client1 gets
from the server the same mtime for f3 as it was for f1 - so, the
server didn't apply mtime after client1 and client2 wrote the data to
the server.

> failed to get a read oplock.   Allowing us to turn off the 1 second timeout
> on metadata/data caching is already possible.

About LookupCacheEnabled, I turned it off when I was running my tests
cache_problem.py and mtime_problem.py but the results were the same.

>
>>> have two opens of the same file from different clients we won't be
>>> caching anyway.
>
> With no oplock we won't be caching writes - we do cache reads
> in some cases (the intent originally was to do this for about 1 second)
> but as you note we can cache longer
>

About writes:
           written = generic_file_aio_write(iocb, iov, nr_segs, pos);
           if (!CIFS_I(inode)->clientCanCacheAll)
                   filemap_fdatawrite(inode->i_mapping);
           return written;

If we don't have write oplock, we always return written value (got
after generic_file_aio_write), but there is no checks if we fails on
filemap_fdatawrite (e.g. if another client has this file opened and
obtains mandatory byte-range lock on it). So, a user always think that
his write is complete successfully, but it can be wrong!

There is another problem with Oplocks: The server sends Oplock break
notification into only one tcon provided by the client. But now in
CIFS code architecture we have no connections between several
connections to the same share - that's why we set Oplock to None only
on one tcon, but another leaves the same (with Oplock Level II). In
this case when we try to read by the second tcon from the file we
think that we have Oplock for reading and read from the cache - it
wrong!

The above situation you can see on the same capture
'mtime_problem.pcap': the server sends Oplock break to None Oplock
only to the FID 0x0002, but no requests for FID 0x0001!

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: mtime_problem.py --]
[-- Type: application/octet-stream, Size: 528 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
close(f)

f1 = open('test1/_test4321_', O_RDWR)
write(f1, 'a')

f2 = open('test2/_test4321_', O_WRONLY)
write(f2, 'x')

f3 = open('test1/_test4321_', O_RDONLY)
print read(f3, 1)

close(f1)
close(f2)
close(f3)

[-- Attachment #3: mtime_problem.pcap --]
[-- Type: application/octet-stream, Size: 11752 bytes --]

Re: CIFS data coherency problem

[Jeff Layton]

On Fri, 10 Sep 2010 11:50:05 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/10 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > Surely it is a serious bug if a server doesn't update the mtime by the
> > time the handle they used is closed.   If a client 1 does open/write/close,
> > then client 2 does open/write/close, client 1 reopening the file should
> > see the updated mtime.   If client 2 had not closed the file yet - it
> > is not clear whether its write and mtime update will be processed
> > first - but we shouldn't be using cached data in that case - client 1 should
> > do an invalidate_mapping when it can't get an oplock (we do that already
> > in seek and mmap via revalidate e.g.).  In any case writes won't be cached
> > in that case - and the simplest change may be to invalidate the inode
> > cached pages in this reopen path - when the mtime/size matches but we
> 
> I mean the situation then we have this file opened by another file handle. E.g.:
> 
> 1) client1 opens file as f1.
> 2) client1 writes 'a' into f1 from the beginning.
> 3) client2 opens file as f2.
> 4) client2 writes 'x' into f2 to the beginning,
> 5) client1 opens file as f3.
> 6) client1 reads from f3 and get 'a'!!! But it must be 'x'!
> 
> I attached this test 'mtime_problem.py' and the capture
> 'mtime_problem.pcap'. On the capture you can see that client1 gets
> from the server the same mtime for f3 as it was for f1 - so, the
> server didn't apply mtime after client1 and client2 wrote the data to
> the server.
> 

I think you may be confusing things a bit. The problem isn't so much
that the server is delaying mtime updates but rather that the client is
buffering up the writes. In that situation the server won't be aware of
changes to the file and hence won't update the mtime.

The real question is...should we expect that the above works without
any sort of locking? The answer for NFS has always been "no" --
concurrent accesses to the same file by multiple clients should be
serialized by posix locks or your results will be inconsistent. To this
end, the NFS client flushes all writes on an unlock and revalidates the
file's metadata on a lock operation.

CIFS is a different beast however and we have to deal with interaction
from clients like windows that expect different behavior. So it may
make sense to always write/read through unless we have an oplock (or a
real file lock).

Dealing with mmap this way though is likely to be extra tricky however.

> > failed to get a read oplock.   Allowing us to turn off the 1 second timeout
> > on metadata/data caching is already possible.
> 
> About LookupCacheEnabled, I turned it off when I was running my tests
> cache_problem.py and mtime_problem.py but the results were the same.
> 

Yeah, I wouldn't expect that to affect much of anything.

> >
> >>> have two opens of the same file from different clients we won't be
> >>> caching anyway.
> >
> > With no oplock we won't be caching writes - we do cache reads
> > in some cases (the intent originally was to do this for about 1 second)
> > but as you note we can cache longer
> >
> 
> About writes:
>            written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>            if (!CIFS_I(inode)->clientCanCacheAll)
>                    filemap_fdatawrite(inode->i_mapping);
>            return written;
> 
> If we don't have write oplock, we always return written value (got
> after generic_file_aio_write), but there is no checks if we fails on
> filemap_fdatawrite (e.g. if another client has this file opened and
> obtains mandatory byte-range lock on it). So, a user always think that
> his write is complete successfully, but it can be wrong!
> 

filemap_fdatawrite starts up a flush of the writes but doesn't wait for
it to complete. The data is cached however. If there's a problem
writing out the data to the server that gets reported at fsync or
close. That's consistent with POSIX. We're not required to report
errors flushing the data until that time.

> There is another problem with Oplocks: The server sends Oplock break
> notification into only one tcon provided by the client. But now in
> CIFS code architecture we have no connections between several
> connections to the same share - that's why we set Oplock to None only
> on one tcon, but another leaves the same (with Oplock Level II). In
> this case when we try to read by the second tcon from the file we
> think that we have Oplock for reading and read from the cache - it
> wrong!
> 
> The above situation you can see on the same capture
> 'mtime_problem.pcap': the server sends Oplock break to None Oplock
> only to the FID 0x0002, but no requests for FID 0x0001!
> 

I see both oplocks being broken:

0x0001: frames 57 and 60
0x0002: frames 65 and 66

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

2010/9/10 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> filemap_fdatawrite starts up a flush of the writes but doesn't wait for
> it to complete. The data is cached however. If there's a problem
> writing out the data to the server that gets reported at fsync or
> close. That's consistent with POSIX. We're not required to report
> errors flushing the data until that time.

In this case we are not flushing all the cached data - we are doing
write (via call from the userspace) and the application wants to know
the right results of it. As I mentioned above, if we have mandatory
byte-range lock from another client on this range of the file, the
writing fails but the application won't know about it!

> I see both oplocks being broken:
>
> 0x0001: frames 57 and 60
> 0x0002: frames 65 and 66
>

The first it is downgrading client1's oplock to Level II when client2
opens the same file (client2 gets Oplock Level II) too), but the
second - downgrading to None - I think it should be for every client
opened the same file. But it the capture:

client2 writes to the file and gets Oplock break to None, but client1
doesn't get it and still think that it can read from a cache (because
it has Oplock Level II).

It is with Samba-4.0.0-alpha11. I tested it with Samba 3.4 and got
another results (the capture is in attachment) - the server downgrades
the oplock of client1 to None when client2 opens the file (client2
gets Oplock None too) - it's right!

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: mtime_problem_samba-3-4-7.pcap --]
[-- Type: application/octet-stream, Size: 13758 bytes --]

Re: CIFS data coherency problem

[Jeff Layton]

On Sat, 11 Sep 2010 00:16:41 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/10 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > filemap_fdatawrite starts up a flush of the writes but doesn't wait for
> > it to complete. The data is cached however. If there's a problem
> > writing out the data to the server that gets reported at fsync or
> > close. That's consistent with POSIX. We're not required to report
> > errors flushing the data until that time.
> 
> In this case we are not flushing all the cached data - we are doing
> write (via call from the userspace) and the application wants to know
> the right results of it. As I mentioned above, if we have mandatory
> byte-range lock from another client on this range of the file, the
> writing fails but the application won't know about it!
> 

So what? Why should the writer know anything about whether it succeeds
before a flush or close occurs? POSIX does not mandate that. If you are
able to report an error at write time, then that's fine. It's not
required however.

> > I see both oplocks being broken:
> >
> > 0x0001: frames 57 and 60
> > 0x0002: frames 65 and 66
> >
> 
> The first it is downgrading client1's oplock to Level II when client2
> opens the same file (client2 gets Oplock Level II) too), but the
> second - downgrading to None - I think it should be for every client
> opened the same file. But it the capture:
> 
> client2 writes to the file and gets Oplock break to None, but client1
> doesn't get it and still think that it can read from a cache (because
> it has Oplock Level II).
> 
> It is with Samba-4.0.0-alpha11. I tested it with Samba 3.4 and got
> another results (the capture is in attachment) - the server downgrades
> the oplock of client1 to None when client2 opens the file (client2
> gets Oplock None too) - it's right!
> 

That's not how I read this:

57: server downgrades oplock on 0x0001 to level 2
60: client drops oplock altogether for 0x0001 (note that it replies
    with oplock level 0)

65: server revokes oplock on 0x0002 altogether
66: client "acks" the oplock break

At that point there are no more oplocks held. Why would the server need
to send another oplock break?
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/11 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> So what? Why should the writer know anything about whether it succeeds
> before a flush or close occurs? POSIX does not mandate that. If you are
> able to report an error at write time, then that's fine. It's not
> required however.

We can write a data in write_end call all the time when we don't have
an oplock for writing. If we get an error we simply returned it in
from write_end and an application gets it.

> That's not how I read this:
>
> 57: server downgrades oplock on 0x0001 to level 2
> 60: client drops oplock altogether for 0x0001 (note that it replies
>    with oplock level 0)
> 65: server revokes oplock on 0x0002 altogether
> 66: client "acks" the oplock break
>
> At that point there are no more oplocks held. Why would the server need
> to send another oplock break?

It (60) is the Oplock Break acknowledge request that's sent by the
client on the server's Oplock Break Notification (57). According to
the spec (page 214, section 2.2.4.32.1 of [MS-CIFS].pdf)
NewOpLockLevel field is used by the server to indicate new oplock
level for the FID. There no words that says that this field is used by
the client in acknowledge. An acknowledge is only used to indicate
that all the dirty data and byte-range locks are flushed to the server
and doesn't bring any other information. It's my understanding of the
spec.

Even more, CIFS client does it according to the spec (file.c, line 2341):

        if (!cfile->closePend && !cfile->oplock_break_cancelled) {
                rc = CIFSSMBLock(0, cifs_sb->tcon, cfile->netfid, 0, 0, 0, 0,

LOCKING_ANDX_OPLOCK_RELEASE, false);
                cFYI(1, "Oplock release rc = %d", rc);
        }

and there is no mention of OplockLevel in CIFSSMBLock call.

So, as you can see above: there is no value that is set to OplockLevel
field of SMB_COM_LOCKING_ANDX command.

That's why I said that client1 continues keeping the level II oplock.

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Jeff Layton]

On Sat, 11 Sep 2010 11:08:15 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/11 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > So what? Why should the writer know anything about whether it succeeds
> > before a flush or close occurs? POSIX does not mandate that. If you are
> > able to report an error at write time, then that's fine. It's not
> > required however.
> 
> We can write a data in write_end call all the time when we don't have
> an oplock for writing. If we get an error we simply returned it in
> from write_end and an application gets it.
> 

I don't understand the above statement. Can you clarify it?

> > That's not how I read this:
> >
> > 57: server downgrades oplock on 0x0001 to level 2
> > 60: client drops oplock altogether for 0x0001 (note that it replies
> >    with oplock level 0)
> > 65: server revokes oplock on 0x0002 altogether
> > 66: client "acks" the oplock break
> >
> > At that point there are no more oplocks held. Why would the server need
> > to send another oplock break?
> 
> It (60) is the Oplock Break acknowledge request that's sent by the
> client on the server's Oplock Break Notification (57). According to
> the spec (page 214, section 2.2.4.32.1 of [MS-CIFS].pdf)
> NewOpLockLevel field is used by the server to indicate new oplock
> level for the FID. There no words that says that this field is used by
> the client in acknowledge. An acknowledge is only used to indicate
> that all the dirty data and byte-range locks are flushed to the server
> and doesn't bring any other information. It's my understanding of the
> spec.
> 
> Even more, CIFS client does it according to the spec (file.c, line 2341):
> 
>         if (!cfile->closePend && !cfile->oplock_break_cancelled) {
>                 rc = CIFSSMBLock(0, cifs_sb->tcon, cfile->netfid, 0, 0, 0, 0,
> 
> LOCKING_ANDX_OPLOCK_RELEASE, false);
>                 cFYI(1, "Oplock release rc = %d", rc);
>         }
> 
> and there is no mention of OplockLevel in CIFSSMBLock call.
> 
> So, as you can see above: there is no value that is set to OplockLevel
> field of SMB_COM_LOCKING_ANDX command.
> 
> That's why I said that client1 continues keeping the level II oplock.
> 

That's just because our client is primitive in this regard. When an
oplock break comes in, it always drops the entire oplock rather than
trying to downgrade it.

Notice that when the client sends the "reply" that it's actually a
request. The client never sends an actual "response" to an oplock
break. What it does is send a new LOCKING_ANDX request to the server
with a Lock Type of oplock break (0x02) and an oplock level of 0.

My interpretation has always been that this "response" from the client
tells the server what new oplock level the client has elected to take.
In this case, it elected to drop the lock altogether so the server has
no need to send a new oplock break.

I'll concede though that the spec is nebulous here. It might be
worthwhile to make a dochelp request to MS on this point. 

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> We can write a data in write_end call all the time when we don't have
>> an oplock for writing. If we get an error we simply returned it in
>> from write_end and an application gets it.
>>
>
> I don't understand the above statement. Can you clarify it?

Yesterday, I sent the patch to the list, that clarifies my idea. I
mean we use do_file_aio_write instead of cifs_file_aio_write call but
in cifs_write_end call (that should actually write data to the
storage) we check for an oplock. If we have the Exclusive oplock for
this file id, we don't write to the server and mark page as dirty. In
other cases we write to the server in cifs_write_end call. You can
find the code that does it in my patch.

> That's just because our client is primitive in this regard. When an
> oplock break comes in, it always drops the entire oplock rather than
> trying to downgrade it.
>
> Notice that when the client sends the "reply" that it's actually a
> request. The client never sends an actual "response" to an oplock
> break. What it does is send a new LOCKING_ANDX request to the server
> with a Lock Type of oplock break (0x02) and an oplock level of 0.
>
> My interpretation has always been that this "response" from the client
> tells the server what new oplock level the client has elected to take.
> In this case, it elected to drop the lock altogether so the server has
> no need to send a new oplock break.
>

You can find the code (misc.c, is_valid_oplock_break call) that
downgrades the oplock for the client according to the field
NewOplockLevel in Oplock Break sent by the server. But then you can
see that in cifs_oplock_break call (file.c) the client simply sends
acknowledge without setting any OplockLevel specific things.

> I'll concede though that the spec is nebulous here. It might be
> worthwhile to make a dochelp request to MS on this point.
>

I agree - without special point in spec it's the open question.

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Jeff Layton]

On Sun, 12 Sep 2010 18:55:05 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> >> We can write a data in write_end call all the time when we don't have
> >> an oplock for writing. If we get an error we simply returned it in
> >> from write_end and an application gets it.
> >>
> >
> > I don't understand the above statement. Can you clarify it?
> 
> Yesterday, I sent the patch to the list, that clarifies my idea. I
> mean we use do_file_aio_write instead of cifs_file_aio_write call but
> in cifs_write_end call (that should actually write data to the
> storage) we check for an oplock. If we have the Exclusive oplock for
> this file id, we don't write to the server and mark page as dirty. In
> other cases we write to the server in cifs_write_end call. You can
> find the code that does it in my patch.
> 

This should probably be a set of patches that outlines each change.
There are 3, AFAICT:

1) it bypasses the cache for reads when we have no oplock. Seems
straightforward.

2) the cache is invalidated on close. So, no caching of data between
open and close. Again ugly for performance but good for strict cache
consistency.

3) write_end is doing a single page synchronous write when there is no
oplock. This is going to be really ugly. Anytime you don't have an
oplock, you'll be doing PAGE_SIZE writes to the server. Why not instead
simply have cifs_file_aio_write do a filemap_write_and_wait or
something if it there is no oplock?

> > That's just because our client is primitive in this regard. When an
> > oplock break comes in, it always drops the entire oplock rather than
> > trying to downgrade it.
> >
> > Notice that when the client sends the "reply" that it's actually a
> > request. The client never sends an actual "response" to an oplock
> > break. What it does is send a new LOCKING_ANDX request to the server
> > with a Lock Type of oplock break (0x02) and an oplock level of 0.
> >
> > My interpretation has always been that this "response" from the client
> > tells the server what new oplock level the client has elected to take.
> > In this case, it elected to drop the lock altogether so the server has
> > no need to send a new oplock break.
> >
> 
> You can find the code (misc.c, is_valid_oplock_break call) that
> downgrades the oplock for the client according to the field
> NewOplockLevel in Oplock Break sent by the server. But then you can
> see that in cifs_oplock_break call (file.c) the client simply sends
> acknowledge without setting any OplockLevel specific things.
> 

Yeah, that looks like a bug. We probably ought to be sending back the
correct oplock break level in the response there.

> > I'll concede though that the spec is nebulous here. It might be
> > worthwhile to make a dochelp request to MS on this point.
> >
> 
> I agree - without special point in spec it's the open question.
> 


-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> This should probably be a set of patches that outlines each change.
> There are 3, AFAICT:

Ok, no problem with this, I will post it soon.

> 1) it bypasses the cache for reads when we have no oplock. Seems
> straightforward.
>
> 2) the cache is invalidated on close. So, no caching of data between
> open and close. Again ugly for performance but good for strict cache
> consistency.
>
> 3) write_end is doing a single page synchronous write when there is no
> oplock. This is going to be really ugly. Anytime you don't have an
> oplock, you'll be doing PAGE_SIZE writes to the server. Why not instead
> simply have cifs_file_aio_write do a filemap_write_and_wait or
> something if it there is no oplock?

I think I didn't understand you in this case. Let's look on the code:

1622 >-------if (!PageUptodate(page) || !CIFS_I(inode)->clientCanCacheAll) {
1623 >------->-------char *page_data;
1624 >------->-------unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
1625 >------->-------int xid;
1626
1627 >------->-------xid = GetXid();
1628 >------->-------/* this is probably better than directly calling
1629 >------->-------   partialpage_write since in this function the
file handle is
1630 >------->-------   known which we might as well>leverage */
1631 >------->-------/* BB check if anything else missing out of ppw
1632 >------->-------   such as updating last write time */
1633 >------->-------page_data = kmap(page);
1634 >------->-------rc = cifs_write(file, page_data + offset, copied, &pos);

We are writing 'copied' bytes from 'offset' of 'page_data'. Why do you
think that we are doing PAGE_SIZE write to the server?

1635 >------->-------/* if (rc < 0) should we set writebehind rc? */
1636 >------->-------kunmap(page);
1637
1638 >------->-------FreeXid(xid);
1639 >-------} else {

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Jeff Layton]

On Sun, 12 Sep 2010 20:16:21 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > This should probably be a set of patches that outlines each change.
> > There are 3, AFAICT:
> 
> Ok, no problem with this, I will post it soon.
> 
> > 1) it bypasses the cache for reads when we have no oplock. Seems
> > straightforward.
> >
> > 2) the cache is invalidated on close. So, no caching of data between
> > open and close. Again ugly for performance but good for strict cache
> > consistency.
> >
> > 3) write_end is doing a single page synchronous write when there is no
> > oplock. This is going to be really ugly. Anytime you don't have an
> > oplock, you'll be doing PAGE_SIZE writes to the server. Why not instead
> > simply have cifs_file_aio_write do a filemap_write_and_wait or
> > something if it there is no oplock?
> 
> I think I didn't understand you in this case. Let's look on the code:
> 
> 1622 >-------if (!PageUptodate(page) || !CIFS_I(inode)->clientCanCacheAll) {
> 1623 >------->-------char *page_data;
> 1624 >------->-------unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
> 1625 >------->-------int xid;
> 1626
> 1627 >------->-------xid = GetXid();
> 1628 >------->-------/* this is probably better than directly calling
> 1629 >------->-------   partialpage_write since in this function the
> file handle is
> 1630 >------->-------   known which we might as well>leverage */
> 1631 >------->-------/* BB check if anything else missing out of ppw
> 1632 >------->-------   such as updating last write time */
> 1633 >------->-------page_data = kmap(page);
> 1634 >------->-------rc = cifs_write(file, page_data + offset, copied, &pos);
> 
> We are writing 'copied' bytes from 'offset' of 'page_data'. Why do you
> think that we are doing PAGE_SIZE write to the server?
> 
> 1635 >------->-------/* if (rc < 0) should we set writebehind rc? */
> 1636 >------->-------kunmap(page);
> 1637
> 1638 >------->-------FreeXid(xid);
> 1639 >-------} else {
> 

write_begin/write_end are called on each page in a write syscall. So if
your application is writing in 64k page-aligned chunks, write_end will
be called 16 times. When you have no oplock with this patch, for each
call to write_end, you're calling cifs_write which will flush each
single page synchronously and only that single page to the server. Your
64k write will take 16 round trips to the server to complete.

What you probably want to do instead is populate the pagecache with the
write contents (as is done today), flush the write and wait on the
result. Optionally, you could then invalidate the cache to free up the
pagecache pages (though you'll need to take care not to race with other
writers).

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> write_begin/write_end are called on each page in a write syscall. So if
> your application is writing in 64k page-aligned chunks, write_end will
> be called 16 times. When you have no oplock with this patch, for each
> call to write_end, you're calling cifs_write which will flush each
> single page synchronously and only that single page to the server. Your
> 64k write will take 16 round trips to the server to complete.
>
> What you probably want to do instead is populate the pagecache with the
> write contents (as is done today), flush the write and wait on the
> result. Optionally, you could then invalidate the cache to free up the
> pagecache pages (though you'll need to take care not to race with other
> writers).

Ok, I understand you. What do you think about the following idea?

1) if we don't have an exclusive oplock, we write the data to the
server before do_sync_write but in cifs_write_end we don't mark the
page as dirty and don't write to the server;
2) if we have an exclusive oplock, we don't write the data before
do_sync_write and mark page as dirty in cifs_write_end (again we don't
write anything to the server in cifs_write_end);

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Steve French]

On Sun, Sep 12, 2010 at 9:55 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> That's just because our client is primitive in this regard. When an
>> oplock break comes in, it always drops the entire oplock rather than
>> trying to downgrade it.
>>
>> Notice that when the client sends the "reply" that it's actually a
>> request. The client never sends an actual "response" to an oplock
>> break. What it does is send a new LOCKING_ANDX request to the server
>> with a Lock Type of oplock break (0x02) and an oplock level of 0.
>>
>> My interpretation has always been that this "response" from the client
>> tells the server what new oplock level the client has elected to take.
>> In this case, it elected to drop the lock altogether so the server has
>> no need to send a new oplock break.
>
> You can find the code (misc.c, is_valid_oplock_break call) that
> downgrades the oplock for the client according to the field
> NewOplockLevel in Oplock Break sent by the server. But then you can
> see that in cifs_oplock_break call (file.c) the client simply sends
> acknowledge without setting any OplockLevel specific things.

We already downgrade olocks.  I don't think we have seen problems with the
downgrade from "cache everything" to "cache only reads" -  I don't think
the client "response" to the oplock downgrade has to set additional flags in
the "response" to the oplock break.

>> I'll concede though that the spec is nebulous here. It might be
>> worthwhile to make a dochelp request to MS on this point.



-- 
Thanks,

Steve

Re: CIFS data coherency problem

[Jeff Layton]

On Sun, 12 Sep 2010 23:14:35 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > write_begin/write_end are called on each page in a write syscall. So if
> > your application is writing in 64k page-aligned chunks, write_end will
> > be called 16 times. When you have no oplock with this patch, for each
> > call to write_end, you're calling cifs_write which will flush each
> > single page synchronously and only that single page to the server. Your
> > 64k write will take 16 round trips to the server to complete.
> >
> > What you probably want to do instead is populate the pagecache with the
> > write contents (as is done today), flush the write and wait on the
> > result. Optionally, you could then invalidate the cache to free up the
> > pagecache pages (though you'll need to take care not to race with other
> > writers).
> 
> Ok, I understand you. What do you think about the following idea?
> 
> 1) if we don't have an exclusive oplock, we write the data to the
> server before do_sync_write but in cifs_write_end we don't mark the
> page as dirty and don't write to the server;
>
> 2) if we have an exclusive oplock, we don't write the data before
> do_sync_write and mark page as dirty in cifs_write_end (again we don't
> write anything to the server in cifs_write_end);
> 

Sorry, I'm not quite following what you mean here. I don't think you
need to mess with cifs_write_end at all.

What I think you basically want to do is replace the filemap_fdatawrite
clause in cifs_file_aio_write with a filemap_write_and_wait. If you
want to get a little more fancy, you could just have it flush the range
of the file actually being written.

After that, you could also consider invalidating the pages in the
mapping (thereby allowing the VM to reclaim them) if you don't have an
oplock.

IOW, you want to write to the pagecache regardless of whether you have
an oplock or not. The oplock only decides whether you flush the data to
the server and wait on the result after writing to the pagecache.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Jeff Layton]

On Sun, 12 Sep 2010 16:19:46 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Sun, Sep 12, 2010 at 9:55 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> >> That's just because our client is primitive in this regard. When an
> >> oplock break comes in, it always drops the entire oplock rather than
> >> trying to downgrade it.
> >>
> >> Notice that when the client sends the "reply" that it's actually a
> >> request. The client never sends an actual "response" to an oplock
> >> break. What it does is send a new LOCKING_ANDX request to the server
> >> with a Lock Type of oplock break (0x02) and an oplock level of 0.
> >>
> >> My interpretation has always been that this "response" from the client
> >> tells the server what new oplock level the client has elected to take.
> >> In this case, it elected to drop the lock altogether so the server has
> >> no need to send a new oplock break.
> >
> > You can find the code (misc.c, is_valid_oplock_break call) that
> > downgrades the oplock for the client according to the field
> > NewOplockLevel in Oplock Break sent by the server. But then you can
> > see that in cifs_oplock_break call (file.c) the client simply sends
> > acknowledge without setting any OplockLevel specific things.
> 
> We already downgrade olocks.  I don't think we have seen problems with the
> downgrade from "cache everything" to "cache only reads" -  I don't think
> the client "response" to the oplock downgrade has to set additional flags in
> the "response" to the oplock break.
> 

If the server doesn't pay attention to that field, then Pavel is
correct and this is a server bug.

Is it legitimate for the client to send an "oplock break" on its own
without the server requesting it first? If so, is the oplock level
field also ignored? How does the server match up an oplock break
"request" to the "response"?

It might be good to ask this on samba-technical (and maybe to ask MS).
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: CIFS data coherency problem

[Steve French]

On Sun, Sep 12, 2010 at 7:15 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Sun, 12 Sep 2010 16:19:46 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Sun, Sep 12, 2010 at 9:55 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8@public.gmane.orgm> wrote:
>> > 2010/9/12 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> >> That's just because our client is primitive in this regard. When an
>> >> oplock break comes in, it always drops the entire oplock rather than
>> >> trying to downgrade it.
>> >>
>> >> Notice that when the client sends the "reply" that it's actually a
>> >> request. The client never sends an actual "response" to an oplock
>> >> break. What it does is send a new LOCKING_ANDX request to the server
>> >> with a Lock Type of oplock break (0x02) and an oplock level of 0.
>> >>
>> >> My interpretation has always been that this "response" from the client
>> >> tells the server what new oplock level the client has elected to take.
>> >> In this case, it elected to drop the lock altogether so the server has
>> >> no need to send a new oplock break.
>> >
>> > You can find the code (misc.c, is_valid_oplock_break call) that
>> > downgrades the oplock for the client according to the field
>> > NewOplockLevel in Oplock Break sent by the server. But then you can
>> > see that in cifs_oplock_break call (file.c) the client simply sends
>> > acknowledge without setting any OplockLevel specific things.
>>
>> We already downgrade olocks.  I don't think we have seen problems with the
>> downgrade from "cache everything" to "cache only reads" -  I don't think
>> the client "response" to the oplock downgrade has to set additional flags in
>> the "response" to the oplock break.
>>
>
> If the server doesn't pay attention to that field, then Pavel is
> correct and this is a server bug.
>
> Is it legitimate for the client to send an "oplock break" on its own
> without the server requesting it first?

No - but I am curious what happens :)

> If so, is the oplock level
> field also ignored? How does the server match up an oplock break
> "request" to the "response"?
>
> It might be good to ask this on samba-technical (and maybe to ask MS).

As this only takes two clients this is fairly easy to test (and prove
Windows to Windows behavior).


-- 
Thanks,

Steve

Re: CIFS data coherency problem

[Pavel Shilovsky]

2010/9/13 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > If the server doesn't pay attention to that field, then Pavel is
> correct and this is a server bug.
>
> Is it legitimate for the client to send an "oplock break" on its own
> without the server requesting it first? If so, is the oplock level

I didn't find a place in spec related to this. And also I didn't see
anything similar in the captures with Windows client.

> field also ignored? How does the server match up an oplock break
> "request" to the "response"?
>
> It might be good to ask this on samba-technical (and maybe to ask MS).

-- 
Best regards,
Pavel Shilovsky.

Re: CIFS data coherency problem

[Pavel Shilovsky]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]

2010/9/13 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> As this only takes two clients this is fairly easy to test (and prove
> Windows to Windows behavior).

I took v2.6.37 CIFS client + strictcache patches and Windows 7 and
found out that Jeff was right. The _should_ set OplockLevel on
OplockBreakAcknowledge. I attached python script that shows the
problem with 2 captures: non-patched.pcap and patched.pcap. 'patched'
one I got after I apply simple patch that set OplockLevel on
OplockBreakAcknoledge,

On the captures, we can see that the server send oplock break to one
client on non-patched variant and to both clients on patched one.
That's why I think our oplock handling code is incorrect now.

I am going to prepare it to post to the list soon.

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: oplock_ack_problem.py --]
[-- Type: text/x-python, Size: 655 bytes --]

#!/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET
import time

f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
close(f)

f1 = open('test1/_test4321_', O_RDWR)
write(f1, 'a')
lseek(f1, 0, 0)
assert('a' == read(f1, 1))

f2 = open('test2/_test4321_', O_WRONLY)
write(f2, 'x')

lseek(f1, 0, 0)

time.sleep(0.1) #need to wait untill a server breaks oplock from the previous write
assert('x' == read(f1, 1))

print 'OK'

close(f1)
close(f2)

[-- Attachment #3: non-patched.pcap --]
[-- Type: application/octet-stream, Size: 13666 bytes --]

[-- Attachment #4: patched.pcap --]
[-- Type: application/octet-stream, Size: 36790 bytes --]