[pnfs] [PATCH 4/6] pnfs: remove spin_unlock from put_unlock_current_layout

[William A. (Andy) Adamson]

On Wed, Jul 23, 2008 at 4:44 PM, Fredric Isaman <iisaman at citi.umich.edu> wrote:
>
>
> On Wed, 23 Jul 2008, andros at netapp.com wrote:
>
>> From: Andy Adamson <andros at localhost.localdomain>
>>
>> Separate the unlocking of the nfs_inode lo_lock from the
>> pnfs_layout_type reference counting.
>>
>> This allows us to get rid of un-needed lock/unlock just to decrement the
>> atomic ref count.
>>
>> Signed-off-by: Andy Adamson<andros at netapp.com>
>> ---
>> fs/nfs/pnfs.c |   32 ++++++++++++++++----------------
>> 1 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 26e4c7f..d5f2b07 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -343,19 +343,22 @@ destroy_layout(struct kref *kref)
>>       struct nfs_client *clp = NFS_SERVER(&nfsi->vfs_inode)->nfs_client;
>>
>>       dprintk("%s: freeing layout %p\n", __func__, lo);
>> +
>> +     spin_lock(&nfsi->lo_lock);
>>       io_ops->free_layout(lo);
>>
>>       nfsi->current_layout = NULL;
>
> Does nfsi->current_layout count as a reference or not?  As is, the patch
> series says not, but this leaves a window where it is possible to do a
> kref_get based on nfsi->current_layout while it is in the process of being
> destroyed.  Once the entire patch series is applied, you can get in this
> situation:
>
> thread1           thread2
> kref_put()
> refcnt==0
>                   pnfs_update_layout() (no lsegs, so...)
>                   get_layout()
> destroy_layout()
> current_layout==NULL
>
>                   pnfs_layout_process()
>                   CRASH

You're right, the above can happen. But, I don't think it's because of
a reference on
the nfsi->current_layout. It's because I removed the spin_lock from
put_unlock_current_layout.
With the lo_lock held, when thread1 puts the refcnt to zero, thread2
would be blocked in pnfs_update_layout from obtaining
nfsi->current_layout, and thread1 would destroy_layout prior to
thread2 getting the layout..

I'll leave put_unlock_current_layout alone.

-->Andy


> Fred
>
>> +     spin_unlock(&nfsi->lo_lock);
>>
>>       /* Unlist the inode.
>> -     * Note that nfsi->lo_lock must be released before getting
>> -     * cl_sem as the latter can sleep
>> -     */
>> -     spin_unlock(&nfsi->lo_lock);
>> +      * Note that nfsi->lo_lock must be released before getting
>> +      * cl_sem as the latter can sleep
>> +      */
>>       down_write(&clp->cl_sem);
>>       spin_lock(&nfsi->lo_lock);
>>       if (!nfsi->current_layout)
>>               list_del_init(&nfsi->lo_inodes);
>> +     spin_unlock(&nfsi->lo_lock);
>>       up_write(&clp->cl_sem);
>> }
>>
>> @@ -363,21 +366,15 @@ destroy_layout(struct kref *kref)
>>  * put and unlock nfs->current_layout
>>  */
>> static inline void
>> -put_unlock_current_layout(struct nfs_inode *nfsi,
>> -                         struct pnfs_layout_type *lo)
>> +put_current_layout(struct pnfs_layout_type *lo)
>> {
>> -     BUG_ON_UNLOCKED_LO(lo);
>>       kref_put(&lo->refcount, destroy_layout);
>> -     spin_unlock(&nfsi->lo_lock);
>> }
>>
>> void
>> pnfs_layout_release(struct pnfs_layout_type *lo)
>> {
>> -     struct nfs_inode *nfsi = NFS_I(lo->inode);
>> -
>> -     spin_lock(&nfsi->lo_lock);
>> -     put_unlock_current_layout(nfsi, lo);
>> +     put_current_layout(lo);
>> }
>>
>> static inline void
>> @@ -754,7 +751,7 @@ static int pnfs_wait_schedule(void *word)
>>  * get, possibly allocate, and lock current_layout
>>  *
>>  * Note: If successful, nfsi->lo_lock is taken and the caller
>> - * must put and unlock current_layout by using put_unlock_current_layout()
>> + * must put and unlock current_layout
>>  * when the returned layout is released.
>>  */
>> static struct pnfs_layout_type *
>> @@ -867,7 +864,8 @@ pnfs_find_get_lseg(struct inode *inode,
>>       range.offset = pos;
>>       range.length = count;
>>       lseg = pnfs_has_layout(lo, &range, 1);
>> -     put_unlock_current_layout(nfsi, lo);
>> +     spin_unlock(&nfsi->lo_lock);
>> +     put_current_layout(lo);
>>       return lseg;
>> }
>>
>> @@ -941,7 +939,8 @@ out:
>> out_put:
>>       if (lsegpp)
>>               *lsegpp = lseg;
>> -     put_unlock_current_layout(nfsi, lo);
>> +     spin_unlock(&nfsi->lo_lock);
>> +     put_current_layout(lo);
>>       goto out;
>> }
>>
>> @@ -1162,7 +1161,8 @@ pnfs_getboundary(struct inode *inode)
>>       lo = get_lock_current_layout(nfsi);;
>>       if (lo) {
>>               stripe_size = policy_ops->get_stripesize(lo);
>> -             put_unlock_current_layout(nfsi, lo);
>> +             spin_unlock(&nfsi->lo_lock);
>> +             put_current_layout(lo);
>>       }
>> out:
>>       return stripe_size;
>> --
>> 1.5.4.1
>>
>> _______________________________________________
>> pNFS mailing list
>> pNFS at linux-nfs.org
>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
>