On Mon, Jul 12, 2010 at 4:30 AM, Benny Halevy <bhalevy@xxxxxxxxxxx> wrote:
> On Jul. 08, 2010, 1:34 +0300, andros@xxxxxxxxxx wrote:
>> From: Andy Adamson <andros@xxxxxxxxxx>
>>
>> Don't wait on a bit if layout is not allocated. Instead, allocate,
>> check for existance, and possibly free.
>>
>> As per new layout refcounting scheme,don't take a reference on the layout.
>>
>> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
>> ---
>> fs/nfs/pnfs.c | 84 +++++++++++++----------------------------------
>> include/linux/nfs_fs.h | 1 -
>> 2 files changed, 23 insertions(+), 62 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 6d435ed..92b7772 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -914,7 +914,6 @@ alloc_init_layout(struct inode *ino)
>> struct pnfs_layout_type *lo;
>> struct layoutdriver_io_operations *io_ops;
>>
>> - BUG_ON(NFS_I(ino)->layout != NULL);
>> io_ops = NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops;
>> lo = io_ops->alloc_layout(ino);
>> if (!lo) {
>> @@ -923,84 +922,47 @@ alloc_init_layout(struct inode *ino)
>> __func__);
>> return NULL;
>> }
>> - spin_lock(&ino->i_lock);
>> lo->refcount = 1;
>> INIT_LIST_HEAD(&lo->lo_layouts);
>> INIT_LIST_HEAD(&lo->segs);
>> seqlock_init(&lo->seqlock);
>> lo->lo_inode = ino;
>> - NFS_I(ino)->layout = lo;
>> - spin_unlock(&ino->i_lock);
>> return lo;
>> }
>>
>> -static int pnfs_wait_schedule(void *word)
>> -{
>> - if (signal_pending(current))
>> - return -ERESTARTSYS;
>> - schedule();
>> - return 0;
>> -}
>> -
>> /*
>> - * get, possibly allocate, and lock current_layout
>> + * Lock and possibly allocate the inode layout
>> *
>> - * Note: If successful, ino->i_lock is taken and the caller
>> - * must put and unlock current_layout when the returned layout is released.
>> + * If successful, ino->i_lock is taken, and the caller must unlock.
>> */
>> static struct pnfs_layout_type *
>> -get_lock_alloc_layout(struct inode *ino)
>> +nfs_lock_alloc_layout(struct inode *ino)
>> {
>> - struct nfs_inode *nfsi = NFS_I(ino);
>> - struct pnfs_layout_type *lo;
>> - int res;
>> + struct pnfs_layout_type *new = NULL;
>>
>> dprintk("%s Begin\n", __func__);
>>
>> spin_lock(&ino->i_lock);
>> - while ((lo = grab_current_layout(nfsi)) == NULL) {
>> + if (NFS_I(ino)->layout == NULL) {
>> spin_unlock(&ino->i_lock);
>
> There's no real need to take the lock here just for checking for NULL,
> only down below before actually committing the newly allocated struct.
> [I'll make this fix)
I'm not taking the lock just to check for NULL. I'm taking the lock
because the function returns a locked i_lock - (or did before you
changed it!)
-->Andy
>
> Benny
>
>> - /* Compete against other threads on who's doing the allocation,
>> - * wait until bit is cleared if we lost this race.
>> - */
>> - res = wait_on_bit_lock(
>> - &nfsi->layout->pnfs_layout_state,
>> - NFS_INO_LAYOUT_ALLOC,
>> - pnfs_wait_schedule, TASK_KILLABLE);
>> - if (res) {
>> - lo = ERR_PTR(res);
>> - break;
>> - }
>> -
>> - /* Was current_layout already allocated while we slept?
>> - * If so, retry get_lock'ing it. Otherwise, allocate it.
>> - */
>> - if (nfsi->layout) {
>> - spin_lock(&ino->i_lock);
>> - continue;
>> + new = alloc_init_layout(ino);
>> + if (new == NULL)
>> + return NULL;
>> + spin_lock(&ino->i_lock);
>> + if (NFS_I(ino)->layout == NULL) {
>> + NFS_I(ino)->layout = new;
>> + new = NULL;
>> }
>> -
>> - lo = alloc_init_layout(ino);
>> - if (lo)
>> - spin_lock(&ino->i_lock);
>> - else
>> - lo = ERR_PTR(-ENOMEM);
>> -
>> - /* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
>> - clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
>> - &nfsi->layout->pnfs_layout_state);
>> - wake_up_bit(&nfsi->layout->pnfs_layout_state,
>> - NFS_INO_LAYOUT_ALLOC);
>> - break;
>> }
>> -
>> -#ifdef NFS_DEBUG
>> - if (!IS_ERR(lo))
>> - dprintk("%s Return %p\n", __func__, lo);
>> - else
>> - dprintk("%s Return error %ld\n", __func__, PTR_ERR(lo));
>> -#endif
>> - return lo;
>> + if (new) {
>> + /* Reference the layout accross i_lock release and grab */
>> + get_layout(NFS_I(ino)->layout);
>> + spin_unlock(&ino->i_lock);
>> + NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops->free_layout(new);
>> + spin_lock(&ino->i_lock);
>> + put_layout(NFS_I(ino)->layout);
>> + }
>> + return NFS_I(ino)->layout;
>> }
>>
>> /*
>> @@ -1088,8 +1050,8 @@ _pnfs_update_layout(struct inode *ino,
>>
>> if (take_ref)
>> *lsegpp = NULL;
>> - lo = get_lock_alloc_layout(ino);
>> - if (IS_ERR(lo)) {
>> + lo = nfs_lock_alloc_layout(ino);
>> + if (lo == NULL) {
>> dprintk("%s ERROR: can't get pnfs_layout_type\n", __func__);
>> goto out;
>> }
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index 7b999a4..005b3ea 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -107,7 +107,6 @@ struct pnfs_layout_type {
>> unsigned long pnfs_layout_state;
>> #define NFS_INO_RO_LAYOUT_FAILED 0 /* get ro layout failed stop trying */
>> #define NFS_INO_RW_LAYOUT_FAILED 1 /* get rw layout failed stop trying */
>> - #define NFS_INO_LAYOUT_ALLOC 2 /* bit lock for layout allocation */
>> #define NFS_INO_LAYOUTCOMMIT 3 /* LAYOUTCOMMIT needed */
>> struct rpc_cred *lo_cred; /* layoutcommit credential */
>> /* DH: These vars keep track of the maximum write range
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
