nfs3_proc_write()

nfs3_proc_write()

[David Chow]

Trond,

I am in the middle of porting the my old nfs_swap code from 2.4.19 to 
2.4.21 .   I notice the interface of nfs3_proc_write() has changed, the 
old code uses a (char *)buffer but the new one uses a (struct page *) . 
When I did the following change, it didn't work.

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,4,20)
    u8               *buffer = (char *)kmap(page);
#endif

 result = NFS_PROTO(inode)->write(inode, cred,
                         &fattr,
                         NFS_RW_SWAP|NFS_RW_SYNC,
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,20)
                         offset, wsize, page, &verf);
#else                         
                         offset, wsize, buffer, &verf);
#endif                         


What happens is it causes an exception in kmap() which is called by 
xdr_kmap() . By tracing the stack with kdb, the path is as follows

nfs3_proc_write-> nfs3_rpc_wrapper->
  rpc_call_sync->rpc_execute->__rpc_execute->call_transmit->xprt_trnasmit->
    do_xprt_transmit->xdr_kmap->kmap(inlined)


By using objdump, the exact instruction which fails in the xdr_kmap() as 
the following

        len = PAGE_CACHE_SIZE;
        if (first_kmap) {
            first_kmap = 0;
 !!!!           iov->iov_base = kmap(*ppage); !!!!
        } else {


The exception is caused by the return page_address(page) . I've also 
tried a trival kmap(page) before triggering nfs3_proc_write() but the 
result is the same. My system only have 128MB RAM so I think it is 
trival that kmap(0 will actually do anything. Looks like the write path 
code of nfs has change a bit which causes this not going to work.  The 
reason why nfs_writepage_sync() can't be used is because the 
nfs_writepage_sync()  assumes page_offset(page) equals to file offset 
but in a swap page this is not true. Please give some hints what is 
going on or which part of code I should look for.

regards,
David Chow

nfs3_proc_write()

[Trond Myklebust]

   > By using objdump, the exact instruction which fails in the xdr_kmap() as 
   > the following

   >        len = PAGE_CACHE_SIZE;
   >        if (first_kmap) {
   >            first_kmap = 0;
   > !!!!           iov->iov_base = kmap(*ppage); !!!!
   >        } else {

  > The exception is caused by the return page_address(page)

You don't want to be writing wsize bytes unless wsize <= PAGE_SIZE. My
guess is that this is why you are seeing the exception.

  > The reason why nfs_writepage_sync() can't be used is because the
  > nfs_writepage_sync() assumes page_offset(page) equals to file
  > offset but in a swap page this is not true. Please give some hints
  > what is going on or which part of code I should look for.

This is not only an assumption in nfs_writepage_sync(), but is also
true in the nfs*_proc_write() code. If you want file offsets that are
not relative to page_offset(page), you'll have to add them as an extra
argument...

Cheers,
  Trond

nfs3_proc_write()

[Trond Myklebust]

>>>>> " " == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

     > You don't want to be writing wsize bytes unless wsize <=
     > PAGE_SIZE. My guess is that this is why you are seeing the
     > exception.

Actually, thinking about it one more time...

You've just thrown the file offset as the argument to ->write()
instead of setting the offset relative to the start of the page,
haven't you?

That will *seriously* confuse the RPC sendmsg() code (which is where
you say the Oops occurs). It just wants to know at what offset within
the page you specified it is supposed to start sending data...

Cheers,
  Trond

Re: nfs3_proc_write()

[David Chow]

Trond Myklebust wrote:

>>>>>>" " == Trond Myklebust <trond.myklebust@fys.uio.no> writes:
>>>>>>            
>>>>>>
>
>     > You don't want to be writing wsize bytes unless wsize <=
>     > PAGE_SIZE. My guess is that this is why you are seeing the
>     > exception.
>
>Actually, thinking about it one more time...
>
>You've just thrown the file offset as the argument to ->write()
>instead of setting the offset relative to the start of the page,
>haven't you?
>
>That will *seriously* confuse the RPC sendmsg() code (which is where
>you say the Oops occurs). It just wants to know at what offset within
>the page you specified it is supposed to start sending data...
>
>Cheers,
>  Trond
>  
>
Yep, I've just discovered and fix the problem by adding two extra 
rpc_nfs_ops called offset_read() and offset_wirte() which allow 
read/write from arbitrary offsets. Some preliminary test has passed.

I think using page_offset(page) to calculate the offset from 
PAGE_CACHE_SIZE is not a pretty good idea. As this interface doesn't 
look like nfs protocol 2/3/4 . Also, it will prohitbit any 
implementation of swap over NFS because page->index has no meaning to 
the actual writing offset of the swapfile.  Anyway, it is only my 
personal recommendation, but looks like it does prohibits some 
functionality. Thanks for your comment.

regards,
David Chow

Re: nfs3_proc_write()

[Trond Myklebust]

>>>>> " " == David Chow <davidchow@shaolinmicro.com> writes:

     > I think using page_offset(page) to calculate the offset from
     > PAGE_CACHE_SIZE is not a pretty good idea. As this interface
     > doesn't look like nfs protocol 2/3/4.

Huh???

     > Also, it will prohitbit any implementation of swap over NFS
     > because page->index has no meaning to the actual writing offset
     > of the swapfile.  Anyway, it is only my personal
     > recommendation, but looks like it does prohibits some
     > functionality. Thanks for your comment.

Those APIs have not been designed for the benefit of NFS swap. They've
been designed for reads/writes to the page cache. That is all we
currently support in the standard kernel.
Why should we keep a load of redundant function arguments in order to
support doing writes from arbitrary pages when there is nothing in the
current kernel that needs this?


You are quite free to define your own specialized read/write interface
to suit the NFS swap patch needs. Despite what you appear to be saying
above, there is nothing that prohibits this...

Cheers,
  Trond