Return-Path: Received: from mail-bw0-f46.google.com ([209.85.214.46]:64910 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755542Ab1BPW4Z convert rfc822-to-8bit (ORCPT ); Wed, 16 Feb 2011 17:56:25 -0500 Received: by bwz15 with SMTP id 15so1140198bwz.19 for ; Wed, 16 Feb 2011 14:56:24 -0800 (PST) In-Reply-To: References: <1297759143-2045-1-git-send-email-andros@netapp.com> <1297759143-2045-11-git-send-email-andros@netapp.com> <4D5C28B3.5010206@panasas.com> <3CAD00F0-0B5E-4A70-B3AA-265E75100DD6@netapp.com> <4D5C2ED8.1030200@panasas.com> Date: Wed, 16 Feb 2011 17:56:24 -0500 Message-ID: Subject: Re: [PATCH pNFS wave 3 Version 2 10/18] NFSv4.1: shift pnfs_update_layout locations From: Fred Isaman To: Benny Halevy Cc: andros@netapp.com, trond.myklebust@netapp.com, linux-nfs@vger.kernel.org, Andy Adamon , Dean Hildebrand , Boaz Harrosh , Oleg Drokin Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Wed, Feb 16, 2011 at 4:09 PM, Fred Isaman wrote: > On Wed, Feb 16, 2011 at 3:08 PM, Benny Halevy wrote: >> On 2011-02-16 14:55, Fred Isaman wrote: >>> >>> On Feb 16, 2011, at 2:42 PM, Benny Halevy wrote: >>> >>>> On 2011-02-15 03:38, andros@netapp.com wrote: >>>>> From: Fred Isaman >>>>> >>>>> Move the pnfs_update_layout call location to nfs_pageio_do_add_request(). >>>>> Grab the lseg sent in the doio function to nfs_read_rpcsetup and attach >>>>> it to each nfs_read_data so it can be sent to the layout driver. >>>>> >>>>> @@ -131,10 +132,12 @@ int nfs_readpage_async(struct nfs_open_context *ctx, struct inode *inode, >>>>> ? ? ? ? ? ? zero_user_segment(page, len, PAGE_CACHE_SIZE); >>>>> >>>>> ? ? nfs_list_add_request(new, &one_request); >>>>> + ? lseg = pnfs_update_layout(inode, ctx, IOMODE_READ); >>>>> ? ? if (NFS_SERVER(inode)->rsize < PAGE_CACHE_SIZE) >>>>> - ? ? ? ? ? nfs_pagein_multi(inode, &one_request, 1, len, 0); >>>>> + ? ? ? ? ? nfs_pagein_multi(inode, &one_request, 1, len, 0, lseg); >>>>> ? ? else >>>>> - ? ? ? ? ? nfs_pagein_one(inode, &one_request, 1, len, 0); >>>>> + ? ? ? ? ? nfs_pagein_one(inode, &one_request, 1, len, 0, lseg); >>>>> + ? put_lseg(lseg); >>>>> ? ? return 0; >>>>> } >>>>> >>>>> @@ -160,7 +163,8 @@ static void nfs_readpage_release(struct nfs_page *req) >>>>> ?*/ >>>>> static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data, >>>>> ? ? ? ? ? ? const struct rpc_call_ops *call_ops, >>>>> - ? ? ? ? ? unsigned int count, unsigned int offset) >>>>> + ? ? ? ? ? unsigned int count, unsigned int offset, >>>>> + ? ? ? ? ? struct pnfs_layout_segment *lseg) >>>>> { >>>>> ? ? struct inode *inode = req->wb_context->path.dentry->d_inode; >>>>> ? ? int swap_flags = IS_SWAPFILE(inode) ? NFS_RPC_SWAPFLAGS : 0; >>>>> @@ -183,6 +187,7 @@ static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data, >>>>> ? ? data->req ? ? ? ? = req; >>>>> ? ? data->inode ? ? ? = inode; >>>>> ? ? data->cred ? ? ? ?= msg.rpc_cred; >>>>> + ? data->lseg ? ? ? ?= get_lseg(lseg); >>>>> >>>>> ? ? data->args.fh ? ? = NFS_FH(inode); >>>>> ? ? data->args.offset = req_offset(req) + offset; >>>>> @@ -240,7 +245,7 @@ nfs_async_read_error(struct list_head *head) >>>>> ?* won't see the new data until our attribute cache is updated. ?This is more >>>>> ?* or less conventional NFS client behavior. >>>>> ?*/ >>>>> -static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags) >>>>> +static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags, struct pnfs_layout_segment *lseg) >>>>> { >>>>> ? ? struct nfs_page *req = nfs_list_entry(head->next); >>>>> ? ? struct page *page = req->wb_page; >>>>> @@ -266,6 +271,8 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne >>>>> ? ? } while(nbytes != 0); >>>>> ? ? atomic_set(&req->wb_complete, requests); >>>>> >>>>> + ? /* We know lseg==NULL */ >> >> Can you provide more details? >> If it's always NULL why bother to pass it in? >> >>>>> + ? lseg = pnfs_update_layout(inode, req->wb_context, IOMODE_READ); >>>>> ? ? ClearPageError(page); >>>>> ? ? offset = 0; >>>>> ? ? nbytes = count; >>>>> @@ -280,12 +287,13 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne >>>>> ? ? ? ? ? ? if (nbytes < rsize) >>>>> ? ? ? ? ? ? ? ? ? ? rsize = nbytes; >>>>> ? ? ? ? ? ? ret2 = nfs_read_rpcsetup(req, data, &nfs_read_partial_ops, >>>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? rsize, offset); >>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rsize, offset, lseg); >>>>> ? ? ? ? ? ? if (ret == 0) >>>>> ? ? ? ? ? ? ? ? ? ? ret = ret2; >>>>> ? ? ? ? ? ? offset += rsize; >>>>> ? ? ? ? ? ? nbytes -= rsize; >>>>> ? ? } while (nbytes != 0); >>>>> + ? put_lseg(lseg); >>>>> >>>>> ? ? return ret; >>>>> >>>>> @@ -300,7 +308,7 @@ out_bad: >>>>> ? ? return -ENOMEM; >>>>> } >>>>> >>>>> -static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags) >>>>> +static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags, struct pnfs_layout_segment *lseg) >>>>> { >>>>> ? ? struct nfs_page ? ? ? ? *req; >>>>> ? ? struct page ? ? ? ? ? ? **pages; >>>>> @@ -320,9 +328,14 @@ static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned >>>>> ? ? ? ? ? ? *pages++ = req->wb_page; >>>>> ? ? } >>>>> ? ? req = nfs_list_entry(data->pages.next); >>>>> + ? if ((!lseg) && list_is_singular(&data->pages)) >>>>> + ? ? ? ? ? lseg = pnfs_update_layout(inode, req->wb_context, IOMODE_READ); >> >> When is lseg NULL and why getting it here works better than in nfs_readpage_async? >> >>>>> >>>>> - ? return nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0); >>>>> + ? ret = nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0, lseg); >>>>> + ? put_lseg(lseg); >>>> >>>> Shouldn't that be done only if pnfs_update_layout was called here? >>>> Otherwise, the caller, nfs_readpage_async puts the lseg it passes down. >>>> >>> >>> You are right there is a problem. ?But it needs to be fixed by removing the put_lseg from nfs_readpage_async. >>> >>> >> >> If we can avoid getting the lseg in one place and putting it in another that would be better. >> >> Benny >> > > I agree, but I don't see how It is possible with the current code, > where the pnfs_update_layout occurs in pg_test. > > Fred > Actually, in this case we can remove both the getting and the putting entirely from nfs_readpage_async, and pass in a NULL lseg. The ->pg_doio functions will handle it correctly. Fred >>>>> + ? return ret; >>>>> out_bad: >>>>> + ? put_lseg(lseg); >>>> >>>> I'd unify the common exit path by doing nfs_async_read_error on the error path >>>> and then goto out for the common code. >>>> >>> >>> OK. >>> >>> Fred >>> >> >