From: "William A. (Andy) Adamson" Subject: Re: [PATCH] NFS: filelayout should use nfs_generic_pg_test Date: Mon, 6 Jun 2011 12:47:29 -0400 Message-ID: References: <1306898310-9229-1-git-send-email-dros@netapp.com> <4DE5D287.603@panasas.com> <09816808-BA7B-4EAF-A18A-866B5A98BF25@netapp.com> <4DE65202.2010502@panasas.com> <1306951621.3873.45.camel@lade.trondhjem.org> <4DE68F47.5080802@panasas.com> <1306956591.3873.69.camel@lade.trondhjem.org> <4DE69C6A.3010204@panasas.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: Trond Myklebust , Weston Andros Adamson , Boaz Harrosh , trond@netapp.com, linux-nfs@vger.kernel.org To: Benny Halevy Return-path: Received: from mail-pw0-f46.google.com ([209.85.160.46]:58075 "EHLO mail-pw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755842Ab1FFQr3 (ORCPT ); Mon, 6 Jun 2011 12:47:29 -0400 Received: by pwi15 with SMTP id 15so2195868pwi.19 for ; Mon, 06 Jun 2011 09:47:29 -0700 (PDT) In-Reply-To: <4DE69C6A.3010204@panasas.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, Jun 1, 2011 at 4:09 PM, Benny Halevy wrote: > On 2011-06-01 22:29, Trond Myklebust wrote: >> On Wed, 2011-06-01 at 22:13 +0300, Benny Halevy wrote: >>> On 2011-06-01 21:07, Trond Myklebust wrote: >>>> On Wed, 2011-06-01 at 17:51 +0300, Benny Halevy wrote: >>>>> I think the following should work: >>>>> >>>>> Benny >>>>> >>>>> git diff --stat -p -M >>>>> fs/nfs/nfs4filelayout.c | 10 ++++++++++ >>>>> 1 files changed, 10 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c >>>>> index 4269088..9f1d445 100644 >>>>> --- a/fs/nfs/nfs4filelayout.c >>>>> +++ b/fs/nfs/nfs4filelayout.c >>>>> @@ -661,6 +661,16 @@ filelayout_pg_test(struct nfs_pageio_descriptor >>>>> *pgio, struct nfs_page *prev, >>>>> u64 p_stripe, r_stripe; >>>>> u32 stripe_unit; >>>>> >>>>> + /* >>>>> + * FIXME: ideally we should be able to coalesce all requests >>>>> + * that are not block boundary aligned, but currently this >>>>> + * is problematic for the case of bsize < PAGE_CACHE_SIZE, >>>>> + * since nfs_flush_multi and nfs_pagein_multi assume you >>>>> + * can have only one struct nfs_page. >>>>> + */ >>>>> + if (desc->pg_bsize < PAGE_SIZE) >>>>> + return 0; >>>>> + >>>>> if (!pnfs_generic_pg_test(pgio, prev, req)) >>>>> return 0; >>>> >>>> So, there are several things that bother me about pnfs_generic_pg_test() >>>> too now that I'm looking more closely at it: >>>> >>>> 1. If the intention is to coalesce 'prev' and 'req', shouldn't we >>>> be asking for a layout with req_offset(prev) instead of >>>> req_offset(req)? >>>> 2. If we're only requesting a layout of length pg_count, don't we >>>> still need to test the layout length that the server actually >>>> returned before we can allow the coalescing? >>>> 3. if (!pgio->lseg), shouldn't we be returning an error of some >>>> sort? Right now we're returning 'true', and allowing the >>>> coalesce to occur. >>>> 4. Furthermore, shouldn't that test guarding the >>>> pnfs_update_layout() call rather be an 'if (pgio->pg_lseg == >>>> NULL)' instead of looking at the values of pg_count and >>>> prev->wb_bytes? >>>> >>> >>> or rather we get the layout for the first page in >>> nfs_pageio_do_add_request when desc->pg_count == 0? >> >> I can live with a desc->pg_init() callback or rather, converting >> pg_test() and pg_doio() into a >> >> struct nfs_pageio_ops { >> int (*pg_init)(struct nfs_pageio_descriptor *desc, struct nfs_page *req); >> bool (*pg_test)(struct nfs_pageio_descriptor *desc, struct nfs_page *prev, struct nfs_page *req); >> int (*pg_doio)(struct nfs_pageio_descriptor *desc); >> }; >> >> and then replacing the two callback functions in the existing struct >> nfs_pageio_descriptor with a single pointer to a 'const struct >> nfs_pageio_ops'... >> > > looks like a good way to do this! Is anyone coding this fix? -->Andy > >>> Then, this lseg would be useful for nfs_flush_multi >>> if we failed to coalesce, or we failed to get a layout >>> altogether we go the nfs path and can reset pg_test to >>> nfs_generic_pg_test. >> >> It would presumably also get rid of all those pnfs_update_layout() calls >> in read.c and write.c. >> > > Yup. > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >