[linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree

[Steve French]

I put Jeff's latest patch in cifs-2.6.git - last chance to review
before I request the push upstream:
http://git.kernel.org/?p=linux/kernel/git/sfrench/cifs-2.6.git;a=commit;h=a98ee8c1c707fe3210b00ef9f806ba8e2bf35504

Running some quick tests with iozone, the performance improvements on
the write subtests were spectacular.   In some cases (presumably due
to caching and journalling effects) beating ext3

On Wed, Nov 26, 2008 at 9:23 AM, Nick Piggin <npiggin at suse.de> wrote:
> On Wed, Nov 26, 2008 at 10:08:57AM -0500, Jeff Layton wrote:
>> On Wed, 26 Nov 2008 14:09:43 +0100
>> Nick Piggin <npiggin at suse.de> wrote:
>>
>>
>> > As to why it can happen, because copy_from_user could take a page fault
>> > on the app's source address (eg. to write(2)).
>>
>> Yep, I figured this out after stumbling across the LWN article.
>>
>> > You _might_ be OK there, but it's not a great idea to SetPageUptodate
>> > first ;) Aside from the problem of the short-copy, SetPageUptodate
>> > actually has a memory barrier in it to ensure the data stored into the
>> > page to bring it uptodate is actually visible before the PageUptodate
>> > flag is. Again, if you are doing DMAs rather than cache coherent stores
>> > to initialise the page, maybe you can get away without that barrier...
>> > But it's just bad practice.
>> >
>>
>> Gotcha. I think the current patch takes care of this (we're using
>> PageChecked to indicate that the uninitialized parts of the page were
>> written to).
>>
>> The problem I suppose is that we could end up getting a short write in
>> write_end. I guess this means that we need to modify the patch a bit
>> further and only set PageUptodate in write_end if copied == len.
>>
>> > > Given that I now understand what AOP_FLAG_UNINTERRUPTIBLE is supposed
>> > > to do, this patch is probably what we need. Running tests on it now.
>> >
>> > That seems pretty reasonable, although keep in mind that
>> > AOP_FLAG_UNINTERRUPTIBLE is not going to be the common case (unless
>> > you're running loop or nfsd or something on the filesystem).
>> >
>> > It would be really nice to figure out a way to avoid the reads in
>> > the interruptible case as well.
>>
>> True. For now though I think we need to start with slow and safe and see
>> if we can optimize it further later...
>
> Yes definitely. Thanks for cleaning up my mess!
>
>
>> > I can't remember the CIFS code very well, but in several of the new
>> > aops conversions I did, I added something like a BUG_ON(!PageUptodate())
>> > in the write_end methods to ensure I wasn't missing some key part of
>> > the logic. It's entirely possible that cifs is almost ready to handle
>> > a !uptodate page in write_end...
>>
>> Well, CIFS is "special". Rather than just updating the pagecache, we
>> can fall back to doing a sync write instead. So I don't think we want
>> to BUG if the page isn't up to date. It's not ideal, but I think it's a
>> situation we can deal with if necessary.
>
> Yes, that would be better. That sync write fallback is quite clever I
> think...
>
>



-- 
Thanks,

Steve