Return-Path: Received: from mail-vw0-f46.google.com ([209.85.212.46]:51535 "EHLO mail-vw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752098Ab1HQJfl (ORCPT ); Wed, 17 Aug 2011 05:35:41 -0400 Received: by vws1 with SMTP id 1so499484vws.19 for ; Wed, 17 Aug 2011 02:35:40 -0700 (PDT) In-Reply-To: <4E4B6A81.2010204@tonian.com> References: <1313197450-4595-1-git-send-email-bergwolf@gmail.com> <4E4ADBA1.1000005@panasas.com> <4E4B6A81.2010204@tonian.com> From: Peng Tao Date: Wed, 17 Aug 2011 17:35:20 +0800 Message-ID: Subject: Re: [PATCH] pnfsblock: init pg_bsize properly To: Benny Halevy , Boaz Harrosh Cc: linux-nfs@vger.kernel.org, Peng Tao Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 Hi, Benny and Boaz, On Wed, Aug 17, 2011 at 3:15 PM, Benny Halevy wrote: > > On 2011-08-17 00:05, Boaz Harrosh wrote: >> On 08/12/2011 06:04 PM, Peng Tao wrote: >>> pg_bsize is server->wsize/rsize by default. We would want to use the lseg length. >>> >> >> Hi >> >> What is the problem you are trying to solve with this patch? >> >> From what I understand the only place that actually cares about >> pg_bsize is nfs_generic_pg_test() which is only used in MDS >> read/write. In the pNFS RW, the LD and pnfs has it's own .pg_test() >> check that should not concern with pg_bsize (Unless for pnfs-files >> which does). So the idea is that pg_bsize is the maximum set by >> MDS server in regard to IO through MDS. And it should not be changed >> by client. >> >> If it is not what you see then we should fix it. But should never >> override MDS wsize/rsize. In pnfs_do_multiple_reads/pnfs_do_multiple_writes, data->mds_ops will be set as desc->pg_rpc_callops, which is determined in nfs_generic_flush/nfs_generic_pagein according to desc->pg_bsize. For blocklayout, we wouldn't want to set data->mds_ops to partial_read/write ops, so I write the patch to use lseg length as pg_bsize. LD can override pg_bsize in pg_init because nfs_pageio_reset_read_mds/nfs_pageio_reset_write_mds will reset it to server rsize/wsize if pnfs is not tried. Sorry that I didn't explain it clearly in the commit log... > > I second that. > > Benny > >> >>> Signed-off-by: Peng Tao >>> --- >>> fs/nfs/blocklayout/blocklayout.c | 20 ++++++++++++++++++-- >>> 1 files changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c >>> index 36648e1..9143e61 100644 >>> --- a/fs/nfs/blocklayout/blocklayout.c >>> +++ b/fs/nfs/blocklayout/blocklayout.c >>> @@ -919,14 +919,30 @@ bl_clear_layoutdriver(struct nfs_server *server) >>> return 0; >>> } >>> >>> +static void bl_pg_init_read(struct nfs_pageio_descriptor *pgio, >>> + struct nfs_page *req) >>> +{ >>> + pnfs_generic_pg_init_read(pgio, req); >>> + if (pgio->pg_lseg) >>> + pgio->pg_bsize = pgio->pg_lseg->pls_range.length; >>> +} >>> + >>> +static void bl_pg_init_write(struct nfs_pageio_descriptor *pgio, >>> + struct nfs_page *req) >>> +{ >>> + pnfs_generic_pg_init_write(pgio, req); >>> + if (pgio->pg_lseg) >>> + pgio->pg_bsize = pgio->pg_lseg->pls_range.length; >>> +} >>> + >>> static const struct nfs_pageio_ops bl_pg_read_ops = { >>> - .pg_init = pnfs_generic_pg_init_read, >>> + .pg_init = bl_pg_init_read, >>> .pg_test = pnfs_generic_pg_test, >> >> I see here that you do not override .pg_test. This is your problem >> look at objio_osd::objio_pg_test() it checks for similar boundaries >> at the objects side. This is where you need to do these checks >> for blocks as well. For blocklayout, we don't need to force each IO under a certain size. Currently (w/ and w/o this patch) the lseg coverage is the only constraint for pagelist length. So pnfs_generic_pg_test is enough for blocklayout. Thanks, Tao >> >>> .pg_doio = pnfs_generic_pg_readpages, >>> }; >>> >>> static const struct nfs_pageio_ops bl_pg_write_ops = { >>> - .pg_init = pnfs_generic_pg_init_write, >>> + .pg_init = bl_pg_init_write, >>> .pg_test = pnfs_generic_pg_test, >> >> Same here >> >>> .pg_doio = pnfs_generic_pg_writepages, >>> }; >> >> Thanks >> Boaz >> -- >> 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 >