Re: [PATCH 2/4] pnfs: allow nfs4_proc_layoutget to sleep on invalid lsegs

[Benny Halevy <bhalevy@panasas.com>]

On 2010-09-14 14:47, Fred Isaman wrote:
> On Tue, Sep 14, 2010 at 5:40 AM, Benny Halevy <bhalevy@panasas.com> wrote:
>> On 2010-09-14 14:00, Fred Isaman wrote:
>>> Note that this waits on any range that overlaps both an outstanding
>>> LAYOUTRETURN and a current layout.  Whereas what you really want is to
>>> wait on any range that overlaps an outstanding LAYOUTRETURN.  One way
>>> to get the desired behavior would be to have LAYOUTRETURN insert an
>>> "invalid" lseg covering its range.
>>
>> What do you mean by "current layout"?
> 
> Consider the case where the server asks for a LAYOUTRETURN of the
> range 0-1000, but the client currently only has lsegs covering 0-500.
> The client should wait for the LAYOUTRETURN to complete before sending
> any LAYOUTGET for the range 500-1000, but there is currently no
> mechanism to do this.
> 

Yes, this could be yet another optimization to avoid
getting NFS4ERR_RECALLCONFLICT.

Benny

> Fred
> 
>> The way it works now is that layout return does mark the layout segments
>> it's about to return as invalid (in pnfs_return_layout_barrier).
>> layout get inserts a valid layout segment only upon completion
>> (in pnfs_layout_process) and it does not yet insert an invalid one
>> before sending the RPC, something we may wan to consider in the future
>> to prevent superfluous parallel gets.
>>
>> Benny
>>
>>>
>>> Fred
>>>
>>>
>>> On Tue, Sep 14, 2010 at 3:53 AM, Benny Halevy <bhalevy@panasas.com> wrote:
>>>> Introduce lo_rpcwaitq and sleep on it while there's an lseg marked invalid,
>>>> i.e. it is being returned (in the future, it could also be layoutget in progress).
>>>>
>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>>>> ---
>>>>  fs/nfs/inode.c         |    1 +
>>>>  fs/nfs/nfs4proc.c      |   25 ++++++++++++++++++++++---
>>>>  fs/nfs/pnfs.c          |   37 ++++++++++++++++++-------------------
>>>>  fs/nfs/pnfs.h          |    3 +++
>>>>  include/linux/nfs_fs.h |    1 +
>>>>  5 files changed, 45 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>>>> index 7621cfc..059bdf7 100644
>>>> --- a/fs/nfs/inode.c
>>>> +++ b/fs/nfs/inode.c
>>>> @@ -1462,6 +1462,7 @@ static inline void nfs4_init_once(struct nfs_inode *nfsi)
>>>>        init_rwsem(&nfsi->rwsem);
>>>>  #ifdef CONFIG_NFS_V4_1
>>>>        init_waitqueue_head(&nfsi->lo_waitq);
>>>> +       rpc_init_wait_queue(&nfsi->lo_rpcwaitq, "pNFS Layout");
>>>>        nfsi->pnfs_layout_suspend = 0;
>>>>        nfsi->layout = NULL;
>>>>  #endif /* CONFIG_NFS_V4_1 */
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 48a763e..f12568d 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -5442,13 +5442,32 @@ nfs4_layoutget_prepare(struct rpc_task *task, void *calldata)
>>>>  {
>>>>        struct nfs4_layoutget *lgp = calldata;
>>>>        struct inode *ino = lgp->args.inode;
>>>> +       struct nfs_inode *nfsi = NFS_I(ino);
>>>>        struct nfs_server *server = NFS_SERVER(ino);
>>>> +       struct pnfs_layout_segment *lseg;
>>>>
>>>>        dprintk("--> %s\n", __func__);
>>>> -       if (nfs4_setup_sequence(server, NULL, &lgp->args.seq_args,
>>>> -                               &lgp->res.seq_res, 0, task))
>>>> +       spin_lock(&ino->i_lock);
>>>> +       lseg = pnfs_has_layout(nfsi->layout, &lgp->args.range);
>>>> +       if (likely(!lseg)) {
>>>> +               spin_unlock(&ino->i_lock);
>>>> +               dprintk("%s: no lseg found, proceeding\n", __func__);
>>>> +               if (!nfs4_setup_sequence(server, NULL, &lgp->args.seq_args,
>>>> +                                        &lgp->res.seq_res, 0, task))
>>>> +                       rpc_call_start(task);
>>>>                return;
>>>> -       rpc_call_start(task);
>>>> +       }
>>>> +       if (!lseg->valid) {
>>>> +               put_lseg_locked(lseg);
>>>> +               spin_unlock(&ino->i_lock);
>>>> +               dprintk("%s: invalid lseg found, waiting\n", __func__);
>>>> +               rpc_sleep_on(&nfsi->lo_rpcwaitq, task, NULL);
>>>> +               return;
>>>> +       }
>>>> +       *lgp->lsegpp = lseg;
>>>> +       spin_unlock(&ino->i_lock);
>>>> +       dprintk("%s: valid lseg found, no rpc required\n", __func__);
>>>> +       rpc_exit(task, NFS4_OK);
>>>>  }
>>>>
>>>>  static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>> index bc4f0c1..2450383 100644
>>>> --- a/fs/nfs/pnfs.c
>>>> +++ b/fs/nfs/pnfs.c
>>>> @@ -430,7 +430,7 @@ destroy_lseg(struct kref *kref)
>>>>        PNFS_LD_IO_OPS(lseg->layout)->free_lseg(lseg);
>>>>  }
>>>>
>>>> -static void
>>>> +void
>>>>  put_lseg_locked(struct pnfs_layout_segment *lseg)
>>>>  {
>>>>        bool do_wake_up;
>>>> @@ -444,8 +444,10 @@ put_lseg_locked(struct pnfs_layout_segment *lseg)
>>>>        do_wake_up = !lseg->valid;
>>>>        nfsi = PNFS_NFS_INODE(lseg->layout);
>>>>        kref_put(&lseg->kref, destroy_lseg);
>>>> -       if (do_wake_up)
>>>> +       if (do_wake_up) {
>>>>                wake_up(&nfsi->lo_waitq);
>>>> +               rpc_wake_up(&nfsi->lo_rpcwaitq);
>>>> +       }
>>>>  }
>>>>
>>>>  void
>>>> @@ -999,7 +1001,7 @@ has_matching_lseg(struct pnfs_layout_segment *lseg,
>>>>  /*
>>>>  * lookup range in layout
>>>>  */
>>>> -static struct pnfs_layout_segment *
>>>> +struct pnfs_layout_segment *
>>>>  pnfs_has_layout(struct pnfs_layout_hdr *lo,
>>>>                struct pnfs_layout_range *range)
>>>>  {
>>>> @@ -1057,22 +1059,20 @@ _pnfs_update_layout(struct inode *ino,
>>>>
>>>>        /* Check to see if the layout for the given range already exists */
>>>>        lseg = pnfs_has_layout(lo, &arg);
>>>> -       if (lseg && !lseg->valid) {
>>>> -               put_lseg_locked(lseg);
>>>> -               /* someone is cleaning the layout */
>>>> -               lseg = NULL;
>>>> -               goto out_unlock;
>>>> -       }
>>>> -
>>>>        if (lseg) {
>>>> -               dprintk("%s: Using cached lseg %p for %llu@%llu iomode %d)\n",
>>>> -                       __func__,
>>>> -                       lseg,
>>>> -                       arg.length,
>>>> -                       arg.offset,
>>>> -                       arg.iomode);
>>>> +               if (lseg->valid) {
>>>> +                       dprintk("%s: Using cached lseg %p for %llu@%llu "
>>>> +                               "iomode %d)\n",
>>>> +                               __func__,
>>>> +                               lseg,
>>>> +                               arg.length,
>>>> +                               arg.offset,
>>>> +                               arg.iomode);
>>>>
>>>> -               goto out_unlock;
>>>> +                       goto out_unlock;
>>>> +               }
>>>> +               /* someone is cleaning the layout */
>>>> +               put_lseg_locked(lseg);
>>>>        }
>>>>
>>>>        /* if get layout already failed once goto out */
>>>> @@ -1097,8 +1097,7 @@ out:
>>>>                nfsi->layout->state, lseg);
>>>>        return;
>>>>  out_unlock:
>>>> -       if (lsegpp)
>>>> -               *lsegpp = lseg;
>>>> +       *lsegpp = lseg;
>>>>        spin_unlock(&ino->i_lock);
>>>>        goto out;
>>>>  }
>>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>>> index ea70385..f7f567e 100644
>>>> --- a/fs/nfs/pnfs.h
>>>> +++ b/fs/nfs/pnfs.h
>>>> @@ -34,6 +34,9 @@ extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp, bool wait);
>>>>  /* pnfs.c */
>>>>  extern const nfs4_stateid zero_stateid;
>>>>
>>>> +void put_lseg_locked(struct pnfs_layout_segment *);
>>>> +struct pnfs_layout_segment *pnfs_has_layout(struct pnfs_layout_hdr *,
>>>> +                                           struct pnfs_layout_range *);
>>>>  void _pnfs_update_layout(struct inode *ino, struct nfs_open_context *ctx,
>>>>        loff_t pos, u64 count, enum pnfs_iomode access_type,
>>>>        struct pnfs_layout_segment **lsegpp);
>>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>>>> index 5bafa95..196c0c6 100644
>>>> --- a/include/linux/nfs_fs.h
>>>> +++ b/include/linux/nfs_fs.h
>>>> @@ -213,6 +213,7 @@ struct nfs_inode {
>>>>        /* pNFS layout information */
>>>>  #if defined(CONFIG_NFS_V4_1)
>>>>        wait_queue_head_t lo_waitq;
>>>> +       struct rpc_wait_queue lo_rpcwaitq;
>>>>        struct pnfs_layout_hdr *layout;
>>>>        time_t pnfs_layout_suspend;
>>>>  #endif /* CONFIG_NFS_V4_1 */
>>>> --
>>>> 1.7.2.2
>>>>
>>>> --
>>>> 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
>>>>
>>