Re: [Nfs-ganesha-devel] reg. drc nested locks

2017-05-03 Thread Matt Benjamin 
I don't see a complete design proposal here.  Can you clarify the fuller 
picture of what you're proposing to do?

Matt

- Original Message -
> From: "Satya Prakash GS" <g.satyaprak...@gmail.com>
> To: "Matt Benjamin" <mbenja...@redhat.com>, "Malahal Naineni" 
> <mala...@gmail.com>,
> nfs-ganesha-devel@lists.sourceforge.net
> Sent: Wednesday, May 3, 2017 4:14:21 PM
> Subject: Re: [Nfs-ganesha-devel] reg. drc nested locks
> 
> Thank you for the quick reply.
> 
> In dupreq_finish, as part of retiring the drc quite a few locks are
> acquired and dropped (per entry). I want to fix a bug where drc retire
> will happen as part of a different function (this will be called from
> free_expired). The existing logic gets carried over to the new
> function and I was thinking that we may not have to acquire and
> release lock so many times.
> 
> Thanks,
> Satya.
> 
> On Thu, May 4, 2017 at 1:21 AM, Matt Benjamin <mbenja...@redhat.com> wrote:
> > Hi Satya,
> >
> > Sorry, my recommendation would be, we do not change locking to be more
> > coarse grained, and in general, should update it in response to an
> > indication that it is incorrect, not to improve readability in the first
> > instance.
> >
> > Regards,
> >
> > Matt
> >
> > - Original Message -
> >> From: "Matt Benjamin" <mbenja...@redhat.com>
> >> To: "Satya Prakash GS" <g.satyaprak...@gmail.com>
> >> Cc: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni"
> >> <mala...@gmail.com>
> >> Sent: Wednesday, May 3, 2017 3:43:06 PM
> >> Subject: Re: [Nfs-ganesha-devel] reg. drc nested locks
> >>
> >> No?
> >>
> >> Matt
> >>
> >> - Original Message -
> >> > From: "Satya Prakash GS" <g.satyaprak...@gmail.com>
> >> > To: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni"
> >> > <mala...@gmail.com>
> >> > Sent: Wednesday, May 3, 2017 3:34:31 PM
> >> > Subject: [Nfs-ganesha-devel] reg. drc nested locks
> >> >
> >> > Hi,
> >> >
> >> > In nfs_dupreq_start and nfs_dupreq_finish when allocating/freeing a
> >> > dupreq_entry we are trying hard to keep both dupreq_q and the rbtree
> >> > in sync acquiring both the partition lock and the drc (t->mtx,
> >> > drc->mtx). This requires dropping and reacquiring locks at certain
> >> > places. Can these nested locks be changed to take locks one after the
> >> > other.
> >> >
> >> > For example at the time of allocation, we could choose to do this -
> >> >
> >> > PTHREAD_MUTEX_lock(>mtx); /* partition lock */
> >> > nv = rbtree_x_cached_lookup(>xt, t, >rbt_k, dk->hk);
> >> > if (!nv) {
> >> > dk->refcnt = 2;
> >> > (void)rbtree_x_cached_insert(>xt, t,
> >> > >rbt_k, dk->hk);
> >> > PTHREAD_MUTEX_unlock(>mtx); /* partition lock */
> >> >
> >> > PTHREAD_MUTEX_lock(>mtx);
> >> > TAILQ_INSERT_TAIL(>dupreq_q, dk, fifo_q);
> >> > ++(drc->size);
> >> > PTHREAD_MUTEX_unlock(>mtx);
> >> > }
> >> >
> >> > I am assuming this would simplify the lock code a lot.
> >> > If there is a case where this would introduce a race please let me know.
> >> >
> >> > Thanks,
> >> > Satya.
> >> >
> >> > --
> >> > Check out the vibrant tech community on one of the world's most
> >> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> >> > ___
> >> > Nfs-ganesha-devel mailing list
> >> > Nfs-ganesha-devel@lists.sourceforge.net
> >> > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
> >> >
> >>
> >> --
> >> Matt Benjamin
> >> Red Hat, Inc.
> >> 315 West Huron Street, Suite 140A
> >> Ann Arbor, Michigan 48103
> >>
> >> http://www.redhat.com/en/technologies/storage
> >>
> >> tel.  734-821-5101
> >> fax.  734-769-8938
> >> cel.  734-216-5309
> >>
> >
> > --
> > Matt Benjamin
> > Red Hat, Inc.
> > 315 West Huron Street, Suite 140A
> > Ann Arbor, Michigan 48103
> >
> > http://www.redhat.com/en/technologies/storage
> >
> > tel.  734-821-5101
> > fax.  734-769-8938
> > cel.  734-216-5309
> 

-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] reg. drc nested locks

2017-05-03 Thread Satya Prakash GS 
Thank you for the quick reply.

In dupreq_finish, as part of retiring the drc quite a few locks are
acquired and dropped (per entry). I want to fix a bug where drc retire
will happen as part of a different function (this will be called from
free_expired). The existing logic gets carried over to the new
function and I was thinking that we may not have to acquire and
release lock so many times.

Thanks,
Satya.

On Thu, May 4, 2017 at 1:21 AM, Matt Benjamin <mbenja...@redhat.com> wrote:
> Hi Satya,
>
> Sorry, my recommendation would be, we do not change locking to be more coarse 
> grained, and in general, should update it in response to an indication that 
> it is incorrect, not to improve readability in the first instance.
>
> Regards,
>
> Matt
>
> - Original Message -
>> From: "Matt Benjamin" <mbenja...@redhat.com>
>> To: "Satya Prakash GS" <g.satyaprak...@gmail.com>
>> Cc: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni" 
>> <mala...@gmail.com>
>> Sent: Wednesday, May 3, 2017 3:43:06 PM
>> Subject: Re: [Nfs-ganesha-devel] reg. drc nested locks
>>
>> No?
>>
>> Matt
>>
>> - Original Message -
>> > From: "Satya Prakash GS" <g.satyaprak...@gmail.com>
>> > To: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni"
>> > <mala...@gmail.com>
>> > Sent: Wednesday, May 3, 2017 3:34:31 PM
>> > Subject: [Nfs-ganesha-devel] reg. drc nested locks
>> >
>> > Hi,
>> >
>> > In nfs_dupreq_start and nfs_dupreq_finish when allocating/freeing a
>> > dupreq_entry we are trying hard to keep both dupreq_q and the rbtree
>> > in sync acquiring both the partition lock and the drc (t->mtx,
>> > drc->mtx). This requires dropping and reacquiring locks at certain
>> > places. Can these nested locks be changed to take locks one after the
>> > other.
>> >
>> > For example at the time of allocation, we could choose to do this -
>> >
>> > PTHREAD_MUTEX_lock(>mtx); /* partition lock */
>> > nv = rbtree_x_cached_lookup(>xt, t, >rbt_k, dk->hk);
>> > if (!nv) {
>> > dk->refcnt = 2;
>> > (void)rbtree_x_cached_insert(>xt, t,
>> > >rbt_k, dk->hk);
>> > PTHREAD_MUTEX_unlock(>mtx); /* partition lock */
>> >
>> > PTHREAD_MUTEX_lock(>mtx);
>> > TAILQ_INSERT_TAIL(>dupreq_q, dk, fifo_q);
>> > ++(drc->size);
>> > PTHREAD_MUTEX_unlock(>mtx);
>> > }
>> >
>> > I am assuming this would simplify the lock code a lot.
>> > If there is a case where this would introduce a race please let me know.
>> >
>> > Thanks,
>> > Satya.
>> >
>> > --
>> > Check out the vibrant tech community on one of the world's most
>> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> > ___
>> > Nfs-ganesha-devel mailing list
>> > Nfs-ganesha-devel@lists.sourceforge.net
>> > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
>> >
>>
>> --
>> Matt Benjamin
>> Red Hat, Inc.
>> 315 West Huron Street, Suite 140A
>> Ann Arbor, Michigan 48103
>>
>> http://www.redhat.com/en/technologies/storage
>>
>> tel.  734-821-5101
>> fax.  734-769-8938
>> cel.  734-216-5309
>>
>
> --
> Matt Benjamin
> Red Hat, Inc.
> 315 West Huron Street, Suite 140A
> Ann Arbor, Michigan 48103
>
> http://www.redhat.com/en/technologies/storage
>
> tel.  734-821-5101
> fax.  734-769-8938
> cel.  734-216-5309

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] reg. drc nested locks

2017-05-03 Thread Matt Benjamin 
Hi Satya,

Sorry, my recommendation would be, we do not change locking to be more coarse 
grained, and in general, should update it in response to an indication that it 
is incorrect, not to improve readability in the first instance.

Regards,

Matt

- Original Message -
> From: "Matt Benjamin" <mbenja...@redhat.com>
> To: "Satya Prakash GS" <g.satyaprak...@gmail.com>
> Cc: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni" 
> <mala...@gmail.com>
> Sent: Wednesday, May 3, 2017 3:43:06 PM
> Subject: Re: [Nfs-ganesha-devel] reg. drc nested locks
> 
> No?
> 
> Matt
> 
> - Original Message -
> > From: "Satya Prakash GS" <g.satyaprak...@gmail.com>
> > To: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni"
> > <mala...@gmail.com>
> > Sent: Wednesday, May 3, 2017 3:34:31 PM
> > Subject: [Nfs-ganesha-devel] reg. drc nested locks
> > 
> > Hi,
> > 
> > In nfs_dupreq_start and nfs_dupreq_finish when allocating/freeing a
> > dupreq_entry we are trying hard to keep both dupreq_q and the rbtree
> > in sync acquiring both the partition lock and the drc (t->mtx,
> > drc->mtx). This requires dropping and reacquiring locks at certain
> > places. Can these nested locks be changed to take locks one after the
> > other.
> > 
> > For example at the time of allocation, we could choose to do this -
> > 
> > PTHREAD_MUTEX_lock(>mtx); /* partition lock */
> > nv = rbtree_x_cached_lookup(>xt, t, >rbt_k, dk->hk);
> > if (!nv) {
> > dk->refcnt = 2;
> > (void)rbtree_x_cached_insert(>xt, t,
> > >rbt_k, dk->hk);
> > PTHREAD_MUTEX_unlock(>mtx); /* partition lock */
> > 
> > PTHREAD_MUTEX_lock(>mtx);
> > TAILQ_INSERT_TAIL(>dupreq_q, dk, fifo_q);
> > ++(drc->size);
> > PTHREAD_MUTEX_unlock(>mtx);
> > }
> > 
> > I am assuming this would simplify the lock code a lot.
> > If there is a case where this would introduce a race please let me know.
> > 
> > Thanks,
> > Satya.
> > 
> > --
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > ___
> > Nfs-ganesha-devel mailing list
> > Nfs-ganesha-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
> > 
> 
> --
> Matt Benjamin
> Red Hat, Inc.
> 315 West Huron Street, Suite 140A
> Ann Arbor, Michigan 48103
> 
> http://www.redhat.com/en/technologies/storage
> 
> tel.  734-821-5101
> fax.  734-769-8938
> cel.  734-216-5309
> 

-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] reg. drc nested locks

2017-05-03 Thread Matt Benjamin 
No?

Matt

- Original Message -
> From: "Satya Prakash GS" 
> To: nfs-ganesha-devel@lists.sourceforge.net, "Malahal Naineni" 
> 
> Sent: Wednesday, May 3, 2017 3:34:31 PM
> Subject: [Nfs-ganesha-devel] reg. drc nested locks
> 
> Hi,
> 
> In nfs_dupreq_start and nfs_dupreq_finish when allocating/freeing a
> dupreq_entry we are trying hard to keep both dupreq_q and the rbtree
> in sync acquiring both the partition lock and the drc (t->mtx,
> drc->mtx). This requires dropping and reacquiring locks at certain
> places. Can these nested locks be changed to take locks one after the
> other.
> 
> For example at the time of allocation, we could choose to do this -
> 
> PTHREAD_MUTEX_lock(>mtx); /* partition lock */
> nv = rbtree_x_cached_lookup(>xt, t, >rbt_k, dk->hk);
> if (!nv) {
> dk->refcnt = 2;
> (void)rbtree_x_cached_insert(>xt, t,
> >rbt_k, dk->hk);
> PTHREAD_MUTEX_unlock(>mtx); /* partition lock */
> 
> PTHREAD_MUTEX_lock(>mtx);
> TAILQ_INSERT_TAIL(>dupreq_q, dk, fifo_q);
> ++(drc->size);
> PTHREAD_MUTEX_unlock(>mtx);
> }
> 
> I am assuming this would simplify the lock code a lot.
> If there is a case where this would introduce a race please let me know.
> 
> Thanks,
> Satya.
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Nfs-ganesha-devel mailing list
> Nfs-ganesha-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
> 

-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel