[pnfs] [PATCH 7/8] pnfs: refactor pnfs_return_layout
Benny Halevy
bhalevy at panasas.com
Wed Mar 12 11:26:58 EDT 2008
On Mar. 12, 2008, 17:16 +0200, Benny Halevy <bhalevy at panasas.com> wrote:
> On Mar. 10, 2008, 20:39 +0200, andros at umich.edu wrote:
>> From: Andy Adamson <andros at umich.edu>
>>
>> Remove pnfs_return_layout_rpc(). Add a recall type parameter to
>> pnfs_return_layout() and simplify calls in pnfs_recall_layout().
>>
>> Signed-off-by: Andy Adamson<andros at umich.edu>
>> ---
>> fs/nfs/callback_proc.c | 19 +++++-----------
>> fs/nfs/inode.c | 2 +-
>> fs/nfs/nfs4proc.c | 2 +-
>> fs/nfs/nfs4state.c | 2 +-
>> fs/nfs/pnfs.c | 53 ++++++++++++-----------------------------------
>> fs/nfs/pnfs.h | 4 +-
>> 6 files changed, 25 insertions(+), 57 deletions(-)
>>
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index 18fa030..b4bbb3d 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -148,11 +148,9 @@ static int pnfs_recall_layout(void *data)
>> {
>> struct inode *inode, *ino;
>> struct nfs_client *clp;
>> - struct nfs_server *server = NULL;
>> struct cb_pnfs_layoutrecallargs rl;
>> struct recall_layout_threadargs *args =
>> (struct recall_layout_threadargs *)data;
>> - struct nfs4_pnfs_layoutreturn_arg lr_arg;
>> int status;
>>
>> daemonize("nfsv4-layoutreturn");
>> @@ -163,7 +161,6 @@ static int pnfs_recall_layout(void *data)
>>
>> clp = args->clp;
>> inode = args->inode;
>> - server = NFS_SERVER(inode);
>> rl = args->rl;
>> args->result = 0;
>> complete(&args->started);
>> @@ -180,7 +177,8 @@ static int pnfs_recall_layout(void *data)
>> */
>>
>> if (rl.cbl_recall_type == RECALL_FILE) {
>> - pnfs_return_layout(inode, &rl.cbl_seg);
>> + /* XXX need to check status on pnfs_return_layout */
>
> we may want to print a debug message on error, but the status
> is not percolating up as we're running in a kthread context.
>
>> + pnfs_return_layout(inode, &rl.cbl_seg, RECALL_FILE);
>> goto out;
>> }
>>
>> @@ -189,21 +187,16 @@ static int pnfs_recall_layout(void *data)
>>
>> /* FIXME: This loop is inefficient, running in O(|s_inodes|^2) */
>> while ((ino = nfs_layoutrecall_find_inode(clp, &rl)) != NULL) {
>> - pnfs_return_layout(ino, &rl.cbl_seg);
>> + /* XXX need to check status on pnfs_return_layout */
>> + pnfs_return_layout(ino, &rl.cbl_seg, RECALL_FILE);
>> iput(ino);
>> }
>>
>> /* send final layoutreturn */
>> - lr_arg.reclaim = 0;
>> - lr_arg.layout_type = server->pnfs_curr_ld->id;
>> - lr_arg.return_type = rl.cbl_recall_type;
>> - lr_arg.lseg = rl.cbl_seg;
>> - lr_arg.inode = inode;
>> -
>> - status = pnfs_return_layout_rpc(server, &lr_arg);
>> + status = pnfs_return_layout(inode, &rl.cbl_seg, rl.cbl_recall_type);
>> if (status)
>> printk(KERN_INFO
>> - "%s: ignoring pnfs_return_layout_rpc status=%d\n",
>> + "%s: ignoring pnfs_return_layout status=%d\n",
>> __func__, status);
>> out:
>> iput(inode);
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index b0c5308..38c9359 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1151,7 +1151,7 @@ void nfs4_clear_inode(struct inode *inode)
>> /* First call standard NFS clear_inode() code */
>> nfs_clear_inode(inode);
>> #ifdef CONFIG_PNFS
>> - pnfs_return_layout(inode, NULL);
>> + pnfs_return_layout(inode, NULL, RECALL_FILE);
>> #endif /* CONFIG_PNFS */
>> }
>> #endif
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 3634814..729e14e 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2160,7 +2160,7 @@ pnfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
>> if (p_ops && p_ops->layoutret_on_setattr) {
>> if (nfsi->layoutcommit_ctx)
>> pnfs_layoutcommit_inode(inode, 0);
>> - pnfs_return_layout(inode, NULL);
>> + pnfs_return_layout(inode, NULL, RECALL_FILE);
>> }
>> }
>> return nfs4_proc_setattr(dentry, fattr, sattr);
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index db7222f..3a4e16e 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -508,7 +508,7 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, mode_t mod
>> range.iomode = nfsi->current_layout->roc_iomode;
>> range.offset = 0;
>> range.length = NFS4_LENGTH_EOF;
>> - pnfs_return_layout(state->inode, &range);
>> + pnfs_return_layout(state->inode, &range, RECALL_FILE);
>> }
>> #endif /* CONFIG_PNFS */
>> nfs4_do_close(path, state, wait);
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index d33fcc7..e4ed81f 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -535,23 +535,29 @@ pnfs_free_layout(struct pnfs_layout_type *lo,
>> }
>>
>> int
>> -pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range)
>> +pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>> + enum pnfs_layoutreturn_type type)
>> {
>> + struct nfs4_pnfs_layoutreturn_arg arg;
>> + struct nfs4_pnfs_layoutreturn_res res;
>> + struct nfs4_pnfs_layoutreturn gdata = {
>> + .args = &arg,
>> + .res = &res,
>> + };
>> struct pnfs_layout_type *lo;
>> struct nfs_inode *nfsi = NFS_I(ino);
>> struct nfs_server *server = NFS_SERVER(ino);
>> - struct nfs4_pnfs_layoutreturn_arg arg;
>> int status;
>>
>> lo = get_lock_current_layout(nfsi);
>
> needed only for type == RETURN_FILE
> (I suggest moving it down below the arg setup)
>
>> - dprintk("%s:Begin layout %p\n", __FUNCTION__, lo);
>> -
>> if (lo == NULL)
>> return 0;
>
> lo may be NULL when doing the final return for RETURN_{FSID,ALL}
>
>>
>> + dprintk("%s:Begin layout %p\n", __FUNCTION__, lo);
>
> s/FUNCTION/func/ please
>
>> +
>> arg.reclaim = 0;
>> arg.layout_type = server->pnfs_curr_ld->id;
>> - arg.return_type = RETURN_FILE;
>> + arg.return_type = type;
>> if (range)
>> arg.lseg = *range;
>> else {
>> @@ -562,44 +568,13 @@ pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range)
>> arg.inode = ino;
>>
>> pnfs_free_layout(lo, &arg.lseg);
>
> This mustn't be called when type != RETURN_FILE.
>
>> - put_unlock_current_layout(nfsi, lo);
>
> needed only for type == RETURN_FILE
> in summary this block can be done after setting up arg, like this:
> if (type == RETURN_FILE) {
> lo = get_lock_current_layout(nfsi);
> if (lo) {
> pnfs_free_layout(lo, &arg.lseg);
> put_unlock_current_layout(nfsi, lo);
> }
> }
>
>> -
>> - status = pnfs_return_layout_rpc(server, &arg);
>> -
>> - dprintk("%s:Exit status %d\n", __FUNCTION__, status);
>> - return status;
>> -}
>> -
>> -int
>> -pnfs_return_layout_rpc(struct nfs_server *server,
>> - struct nfs4_pnfs_layoutreturn_arg *argp)
>> -{
>> - int status;
>> - struct nfs4_pnfs_layoutreturn_res res;
>> - struct nfs4_pnfs_layoutreturn gdata = {
>> - .args = argp,
>> - .res = &res,
>> - };
>> - dprintk("%s:Begin\n", __FUNCTION__);
>>
>> - /* XXX Need to setup the sequence */
>> -/*
>> - status = server->nfs_client->rpc_ops->setup_sequence(
>> - server->session,
>> - argp->minorversion_info,
>> - res.minorversion_info)
>> - if (status)
>> - goto out;
>> -*/
>> - /* Return layout to server */
>> + spin_unlock(&nfsi->lo_lock);
>
> only for type == RETURN_FILE.
> (there was a reason why I originally split the return function from
> the return_rpc :)
>
>> status = server->nfs_client->rpc_ops->pnfs_layoutreturn(&gdata);
>> + spin_lock(&nfsi->lo_lock);
>
> ditto
>
>>
>> -/*
>> - server->nfs_client->rpc_ops->sequence_done(server->session,
>> - res.minorversion_info, status);
>> + put_unlock_current_layout(nfsi, lo);
>
> ditto
> hmm, actually rather than doing that:
> unlock()
> do the rpc
> lock()
> put_unlock()
>
> the example I gave above already put_unlock's the layout at
> the right time and just for RETURN_FILE so there's no need
> to joggle the lock anymore around the rpc call.
That works for this patch only.
I the next patch I see that you actually want to keep the layout around
to update the layout stateid in the async rpc done method.
so just to the get_lock part for the RETURN_FILE case, call free_layout,
and unlock the layout.
(for now, so the segment wouldn't be found, in the future I'd like to
keep it hashed and use it as a rendezvous point, i.e. flag it
as being returned so that callers that need it will wait for layout return
to finish before attempting to get it again)
and then in the done function grab the lock do what you have to do
(in the future actually unhash the segment, wakeup waiters, and
free the segment), and put_unlock the layout.
>
> Benny
>
>>
>> -out:
>> -*/
>> dprintk("%s:Exit status %d\n", __FUNCTION__, status);
>> return status;
>> }
>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>> index db25489..0e0e678 100644
>> --- a/fs/nfs/pnfs.h
>> +++ b/fs/nfs/pnfs.h
>> @@ -20,8 +20,8 @@ int pnfs_update_layout(struct inode *ino, struct nfs_open_context *ctx,
>> size_t count, loff_t pos, enum pnfs_iomode access_type,
>> struct pnfs_layout_segment **lsegpp);
>>
>> -int pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range);
>> -int pnfs_return_layout_rpc(struct nfs_server *server, struct nfs4_pnfs_layoutreturn_arg *argp);
>> +int pnfs_return_layout(struct inode *, struct nfs4_pnfs_layout_segment *,
>> + enum pnfs_layoutreturn_type);
>> void set_pnfs_layoutdriver(struct super_block *sb, struct nfs_fh *fh, u32 id);
>> void unmount_pnfs_layoutdriver(struct super_block *sb);
>> int pnfs_use_read(struct inode *inode, ssize_t count);
>
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
More information about the pNFS
mailing list