[pnfs] [PATCH 0/6] pNFS file layout write path - help with testing

[William A. (Andy) Adamson]

Hi Benny

On 1/21/08, Benny Halevy <bhalevy at panasas.com> wrote:
>
> Andy, I submitted the patchset onto the andros-write-path branch.
> I pass connectathon tests over it with panlayout (with the 3 fixes I
> submitted today:
>
> http://git.linux-nfs.org/?p=bhalevy/linux-pnfs.git;a=commitdiff;h=05962a25af175d0a7a8faeb686242200a290cb5d
>
> http://git.linux-nfs.org/?p=bhalevy/linux-pnfs.git;a=commitdiff;h=3e6b91897aefaffdea5a33867137c6d7f3194a2c
>
> http://git.linux-nfs.org/?p=bhalevy/linux-pnfs.git;a=commitdiff;h=c5d8cff478aa7092453dd70cda1fd70f003829c5


thanks benny. good fixes.

However, there seems to be something fishy in pnfs_writeback_done_update
> that's being called for all DS writes.


fishy indeed.

        if (resp->verf->committed == NFS_FILE_SYNC ||
>
> Shouldn't that be != NFS_FILE_SYNC?


no  - In the async rpc case, we have to successfully COMMIT before calling
LAYOUTCOMMIT.

            (data->how == FLUSH_STABLE && data->call_ops == NULL))
>                 pnfs_need_layoutcommit(nfsi, argp->context);
>
> I don't quite understand this conditional term...
> 1. can data->call_ops be NULL?


in the 2.6.18.3 code, this function was called from
pnfs_writeback_done_norpc(), which had a NULL call_ops.

2. why does it matter if the write was stable or not?
> Do you assume that if the DS write was FLUSH_STABLE metadata
> on the MDS is already up-to-date and therefore no layoutcommit is needed?
> If so that should be implemented as !((data->how & FLUSH_STABLE)


i believe these tests were for the norpc case.
here is the 2.6.18.3 function.

void pnfs_writeback_done_update(struct nfs_write_data *data)
{
  if(resp->count > 0 && pnfs_use_write(data->inode, argp->count)) {
                pnfs_update_last_write(nfsi, argp->offset, resp->count);

                if (!pnfs_use_nfsv4_wproto(data->inode, argp->count) ||
                    ((resp->verf->committed == NFS_FILE_SYNC)  ||
                    (data->how == FLUSH_STABLE && data->call_ops == NULL)))
                        pnfs_need_layoutcommit(nfsi, argp->context);
        }
}

For the new write path, this test is redundant.
if(resp->count > 0 && pnfs_use_write(data->inode, argp->count)) {

pnfs_writeback_done_update is only called on a DS write from
pnfs4_write_done() when task->tk_status is > 0 : e.g. when resp->count is >
0.
pnfs_use_write also checks for count > 0, and performs the threshold test -
which has already been performed at page coalese time.
so we know that the count is positive and that this is a DS rpc.

This test
            if (!pnfs_use_nfsv4_wproto(data->inode, argp->count) ||

is useless for the filelayout, as it returns true if the layout type is
LAYOUT_NFSV4_FILES. pnfs_need_layoutcommit should not be called in the async
rpc write done path.

The inline patch (also emailed to you) does the right thing which is to
remove pnfs_writeback_done_update() and call pnfs_update_last_write() in
it's place. (no pnfs_need_layoutcommit).

the pnfs_need_layoutcommit for file layout is correctly called in
pnfs4_commit_done.

With this patch and your fixes, I passed the basic, special, and the general
- except for the write/read 30 MB  test where the DS (running an old kernel)
crashed.

-->Andy



Benny
>
> On Jan. 18, 2008, 17:01 +0200, "William A. (Andy) Adamson" <andros at citi.umich.edu>
> wrote:
> > The first 5 patches are a resend - benny didn't receive them.
> >
> > -->Andy
> >
> > ---------- Forwarded message ----------
> > From: William A. (Andy) Adamson < andros at citi.umich.edu>
> > Date: Jan 17, 2008 2:22 PM
> > Subject: [PATCH 0/5] pNFS file layout write path - help with testing
> > To: pnfs at linux-nfs.org
> >
> > OK - I've responded to Benny's code review. Here are the refactored
> patches,
> > they exhibit the same behaviour - failing the special cthon test at
> >
> > second check for lost reply on non-idempotent requests
> > testing 50 idempotencies in directory "testdir"
> > stat 1: bad file type/size 0100611/137438953473
> >
> > and failing the general connectathon tests on the GPFS DSserver:
> >
> > Four simultaneous large compiles test - fails with Stale NFS Handle.
> >
> > Please help debug....
> >
> > Thanks
> > -->Andy
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > pNFS mailing list
> > pNFS at linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://linux-nfs.org/pipermail/pnfs/attachments/20080121/a10868de/attachment.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pnfs-remove-pnfs_writeback_done_update.patch
Type: text/x-patch
Size: 3016 bytes
Desc: not available
Url : http://linux-nfs.org/pipermail/pnfs/attachments/20080121/a10868de/attachment.bin