About function bio_detain in file dm-cache-target.c

About function bio_detain in file dm-cache-target.c

[majianpeng]

Hi all,
    I think bio_detain(in dm-cache-target.c) may cause some bug.
>static int bio_detain(struct cache *cache, dm_oblock_t oblock,
>                      struct bio *bio, struct dm_bio_prison_cell *cell_prealloc,
>                      cell_free_fn free_fn, void *free_context,
>                      struct dm_bio_prison_cell **cell_result)
>{
>       int r;
>        struct dm_cell_key key;

>        build_key(oblock, &key);
>        r = dm_bio_detain(cache->prison, &key, bio, cell_prealloc, cell_result);
>       if (r)
>                free_fn(free_context, cell_prealloc);

>     return r;
>}
If the returned value of dm_bio_detain is not zero, it indicated cell existing.So it should defer bio.
But in dm_bio_detain-->bio_detain--->__bio->detain
>        cell = __search_bucket(prison->cells + hash, key);
>        if (cell) {
>               if (inmate)
>                        bio_list_add(&cell->bios, inmate);
>                *cell_result = cell;
>                return 1;
>        }  
If found the cell, it will add bio to ->bios of cell.

Maybe bio_detain(in dm-cache) is like:
    r=dm_get_cell();
    if (!r)
      dm_bio_detain


Thanks!
Jianpeng Ma

Re: About function bio_detain in file dm-cache-target.c

[thornber]

On Tue, Mar 12, 2013 at 03:47:28PM +0800, majianpeng wrote:
> Hi all,
>     I think bio_detain(in dm-cache-target.c) may cause some bug.

Hi Jianpeng,

I don't really see what you're getting at, but will try and explain
bio_detain a bit.

The bio prison contains a bunch of cells.  When you call bio_detain
the following steps are performed:

    i) look up the cell
    ii) if the cell exists add the bio to that cell
    iii) if there is no cell create it and insert the bio as the holder

It does do more than I'd like a single function to do, but we need
this group of operations to be atomic to avoid race conditions.

Now if you look at cache_map() where bio_detain is used:

        r = bio_detain(cache, block, bio, cell,
                       (cell_free_fn) free_prison_cell,
                       cache, &cell);
        if (r) {
                if (r < 0)
                        defer_bio(cache, bio);

                return DM_MAPIO_SUBMITTED;
        }

We call bio_detain checking the return value, there are 3 cases:

   0 - this bio has been inserted as the holder, we try and process it
       immediately and do not pass it to the worker thread.

   1 - the bio has been inserted into the cell, but it's not the
       holder.  When the holder of that particular cell completes it will
       release all bios in the cell, putting them on the deferred list for
       processing by the worker thread.

   < 0 - we couldn't complete, hand over to the worker thread who has
         a bit more leeway regarding memory allocation etc.

In typical use, a cell is only held while we decide what to do with the
bio (see calls to cell_defer in cache_map).  But sometimes we decide
to move a block from/to the cache, in which case the bio_prison
prevents any io going to that block while the migration is pending.

I hope that helps,

- Joe