zfs code - Sep 2007 - newbie question about a dbuf-arc eviction race

Hi,

Can a dbuf be in DB_CACHED state, db_holds == 0,
b_efunc != NULL while its db_buf is put on the
eviction list ?  From an ASSERT in dbuf_do_evict(),
it appears that it can. If it can, I am wondering what
is preventing the following race

dbuf_hold_impl()
     db = dbuf_find(dn, level, blkid);

     ...
     if (db->db_buf && refcount_is_zero(&db->db_holds)) {
                 arc_buf_add_ref(db->db_buf, db);
                 /*
                  * The above just returns as db_buf is on the
                  * eviction list. Now, suppose arc_do_user_evicts()
                  * selects this buf and calls dbuf_do_evict().
                  * Nothing really stops this function.
                  *
                  * Now there is a race between dbuf_do_evict() and
                  * the following code
                  */
                 if (db->db_buf->b_data == NULL) {
                         ....
     ....
     }

Sorry if I overlooked something that is very obvious.

Thanks
Bhaskar

Bhaskar,

In the code below, dbuf_find() will return with db_mtx locked which will 
prevent dbuf_do_evict() from proceedin as it will block waiting on this 
mutex:

dbuf_do_evict(void *private)
{
         if (!MUTEX_HELD(&db->db_mtx))
                 mutex_enter(&db->db_mtx);

Thanks,
George

Bhaskar Sarkar wrote:
> Hi,
> 
> Can a dbuf be in DB_CACHED state, db_holds == 0,
> b_efunc != NULL while its db_buf is put on the
> eviction list ?  From an ASSERT in dbuf_do_evict(),
> it appears that it can. If it can, I am wondering what
> is preventing the following race
> 
> dbuf_hold_impl()
>      db = dbuf_find(dn, level, blkid);
> 
>      ...
>      if (db->db_buf && refcount_is_zero(&db->db_holds)) {
>                  arc_buf_add_ref(db->db_buf, db);
>                  /*
>                   * The above just returns as db_buf is on the
>                   * eviction list. Now, suppose arc_do_user_evicts()
>                   * selects this buf and calls dbuf_do_evict().
>                   * Nothing really stops this function.
>                   *
>                   * Now there is a race between dbuf_do_evict() and
>                   * the following code
>                   */
>                  if (db->db_buf->b_data == NULL) {
>                          ....
>      ....
>      }
> 
> Sorry if I overlooked something that is very obvious.
> 
> Thanks
> Bhaskar
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code

Thanks George. The code in dbuf_do_evict() will not
attempt to get the mutex again as it is held already.
This mutex_enter() is simply to avoid acquiring it twice
and not a synchonization lock.

Bhaskar
> 
> In the code below, dbuf_find() will return with db_mtx locked which will 
> prevent dbuf_do_evict() from proceedin as it will block waiting on this 
> mutex:
> 
> dbuf_do_evict(void *private)
> {
>          if (!MUTEX_HELD(&db->db_mtx))
>                  mutex_enter(&db->db_mtx);
> 
> Thanks,
> George
> 
> Bhaskar Sarkar wrote:
>> Hi,
>>
>> Can a dbuf be in DB_CACHED state, db_holds == 0,
>> b_efunc != NULL while its db_buf is put on the
>> eviction list ?  From an ASSERT in dbuf_do_evict(),
>> it appears that it can. If it can, I am wondering what
>> is preventing the following race
>>
>> dbuf_hold_impl()
>>      db = dbuf_find(dn, level, blkid);
>>
>>      ...
>>      if (db->db_buf &&
refcount_is_zero(&db->db_holds)) {
>>                  arc_buf_add_ref(db->db_buf, db);
>>                  /*
>>                   * The above just returns as db_buf is on the
>>                   * eviction list. Now, suppose arc_do_user_evicts()
>>                   * selects this buf and calls dbuf_do_evict().
>>                   * Nothing really stops this function.
>>                   *
>>                   * Now there is a race between dbuf_do_evict() and
>>                   * the following code
>>                   */
>>                  if (db->db_buf->b_data == NULL) {
>>                          ....
>>      ....
>>      }
>>
>> Sorry if I overlooked something that is very obvious.
>>
>> Thanks
>> Bhaskar
>> _______________________________________________
>> zfs-code mailing list
>> zfs-code at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/zfs-code
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code

Bhaskar,

In your scenario the mutex would be held by the other thread in 
dbuf_hold_impl(). So the caller of the dbuf_do_evict() would not own the 
mutex and would have to acquire it. It''s quite possible that the 
eviction thread could have acquired the mutex earlier in the call path. 
Regardless, it would prevent the race you mentioned below.

Thanks,
George

Bhaskar Sarkar wrote:
> Thanks George. The code in dbuf_do_evict() will not
> attempt to get the mutex again as it is held already.
> This mutex_enter() is simply to avoid acquiring it twice
> and not a synchonization lock.
> 
> Bhaskar
> 
>> In the code below, dbuf_find() will return with db_mtx locked which
will
>> prevent dbuf_do_evict() from proceedin as it will block waiting on this
>> mutex:
>>
>> dbuf_do_evict(void *private)
>> {
>>          if (!MUTEX_HELD(&db->db_mtx))
>>                  mutex_enter(&db->db_mtx);
>>
>> Thanks,
>> George
>>
>> Bhaskar Sarkar wrote:
>>> Hi,
>>>
>>> Can a dbuf be in DB_CACHED state, db_holds == 0,
>>> b_efunc != NULL while its db_buf is put on the
>>> eviction list ?  From an ASSERT in dbuf_do_evict(),
>>> it appears that it can. If it can, I am wondering what
>>> is preventing the following race
>>>
>>> dbuf_hold_impl()
>>>      db = dbuf_find(dn, level, blkid);
>>>
>>>      ...
>>>      if (db->db_buf &&
refcount_is_zero(&db->db_holds)) {
>>>                  arc_buf_add_ref(db->db_buf, db);
>>>                  /*
>>>                   * The above just returns as db_buf is on the
>>>                   * eviction list. Now, suppose
arc_do_user_evicts()
>>>                   * selects this buf and calls dbuf_do_evict().
>>>                   * Nothing really stops this function.
>>>                   *
>>>                   * Now there is a race between dbuf_do_evict() and
>>>                   * the following code
>>>                   */
>>>                  if (db->db_buf->b_data == NULL) {
>>>                          ....
>>>      ....
>>>      }
>>>
>>> Sorry if I overlooked something that is very obvious.
>>>
>>> Thanks
>>> Bhaskar
>>> _______________________________________________
>>> zfs-code mailing list
>>> zfs-code at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/zfs-code
>> _______________________________________________
>> zfs-code mailing list
>> zfs-code at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/zfs-code
> 
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code