Don't use the bi_next field for the holder of a cell.
On Thu, Feb 02 2012 at 11:39am -0500,
Joe Thornber <ejt@redhat.com> wrote:
> The holder can be passed down to lower devices which may want to use
> bi_next themselves. Also added BUG_ON checks to confirm fix.
Aside from not (ab)using bi_next; do any other patches in the series
depend on this patch? I don't think so but figured I'd explicitly ask.
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index c308757..6ef03a2 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -220,8 +220,8 @@ static struct cell *__search_bucket(struct hlist_head *bucket,
> * This may block if a new cell needs allocating. You must ensure that
> * cells will be unlocked even if the calling thread is blocked.
> *
> - * Returns the number of entries in the cell prior to the new addition
> - * or < 0 on failure.
> + * Returns 1 if the cell was already held, 0 if @inmate is the new holder,
> + * or < 0 on error.
> */
> static int bio_detain(struct bio_prison *prison, struct cell_key *key,
> struct bio *inmate, struct cell **ref)
bio_detain() never returns < 0, so I've updated the above comment block.
So in this case where there is no existing inmate in the cell, and
you're allocating the cell, you aren't adding the lone inmate (the
holder) to the cell->bios. But you did previously [1].
[1] So you only added it in all cases before because you were looking to
get bio->bi_next to reflect the holder? Thing is bio_list_add()
will set: bio->bi_next = NULL; -- so I'm not seeing how overloading
bi_next to imply holder was reliable.
I'll need to look closer at the bigger picture of how a cell is used;
but it is clear that cell->holder isn't on the cell->bios bio_list now.
__cell_release() assumes @inmates is a properly initialized bio_list.
So why the check for inmates != NULL?
And why is this bio_list_add_head() outside that inmates != NULL check?
But most important question: why is it so important to put the
cell->holder at the head of the @inmates list?
Anyway, __cell_release() is the only place where cell->holder is
actually used -- would be worthwhile to to add a comment above it's
bio_list_add_head() call to document _why_ it is important to add the
holder to the head there.
Especially considering @inmates _could_ already be non-empty, so you're
appending the cell->bios to the end of the @inmates list
(e.g. pool->deferred_bios), but then putting the holder of the cell at
the head of the @inmates list.. leaving the cell's holder disjoint from
it's other cell members... why? Was this intended?
I've refreshed this patch here:
http://people.redhat.com/msnitzer/patches/upstream/dm-thinp-3.3/0004-dm-thin-don-t-use-the-bi_next-field-for-the-holder-o.patch
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
02-10-2012, 01:55 PM
Joe Thornber
Don't use the bi_next field for the holder of a cell.
On Tue, Feb 07, 2012 at 06:18:21PM -0500, Mike Snitzer wrote:
> On Thu, Feb 02 2012 at 11:39am -0500,
> Joe Thornber <ejt@redhat.com> wrote:
>
> > The holder can be passed down to lower devices which may want to use
> > bi_next themselves. Also added BUG_ON checks to confirm fix.
>
> Aside from not (ab)using bi_next; do any other patches in the series
> depend on this patch? I don't think so but figured I'd explicitly ask.
No. I found the issue while chasing a discard problem.
> bio_detain() never returns < 0, so I've updated the above comment block.
Fine.
>
> > @@ -256,21 +256,25 @@ static int bio_detain(struct bio_prison *prison, struct cell_key *key,
> >
> > cell->prison = prison;
> > memcpy(&cell->key, key, sizeof(cell->key));
> > - cell->count = 0;
> > + cell->holder = inmate;
> > bio_list_init(&cell->bios);
> > hlist_add_head(&cell->list, prison->cells + hash);
> > + r = 0;
>
> So in this case where there is no existing inmate in the cell, and
> you're allocating the cell, you aren't adding the lone inmate (the
> holder) to the cell->bios. But you did previously [1].
Correct, the bio that hold the cell previously went into the
cell->bios list. But these ios are sometimes issued before the cell
is released (eg, if the io totally overwrites a data block, we can use
it to 'zero' or 'copy'). The bio is still retained in the cell
however, just in the 'holder' field.
>
> > +
> > + } else {
> > + mempool_free(cell2, prison->cell_pool);
> > + cell2 = NULL;
> > + r = 1;
> > + bio_list_add(&cell->bios, inmate);
> > }
> > - }
> >
> > - r = cell->count++;
> > - bio_list_add(&cell->bios, inmate);
>
> [1] So you only added it in all cases before because you were looking to
> get bio->bi_next to reflect the holder? Thing is bio_list_add()
> will set: bio->bi_next = NULL; -- so I'm not seeing how overloading
> bi_next to imply holder was reliable.
Hmm, not really. The holder was known outside the cell (release_except ...)
> I'll need to look closer at the bigger picture of how a cell is used;
> but it is clear that cell->holder isn't on the cell->bios bio_list now.
Correct.
>
> > @@ -286,6 +290,7 @@ static void __cell_release(struct cell *cell, struct bio_list *inmates)
> > if (inmates)
> > bio_list_merge(inmates, &cell->bios);
> >
> > + bio_list_add_head(inmates, cell->holder);
> > mempool_free(cell, prison->cell_pool);
> > }
>
> __cell_release() assumes @inmates is a properly initialized bio_list.
> So why the check for inmates != NULL?
This is a bug, that bio_list_add_head() should be inside the if too, thx.
> And why is this bio_list_add_head() outside that inmates != NULL check?
exactly.
> But most important question: why is it so important to put the
> cell->holder at the head of the @inmates list?
I don't want to reorder the ios. The holder was definitely the first io originally.
> Especially considering @inmates _could_ already be non-empty, so you're
> appending the cell->bios to the end of the @inmates list
> (e.g. pool->deferred_bios), but then putting the holder of the cell at
> the head of the @inmates list.. leaving the cell's holder disjoint from
> it's other cell members... why? Was this intended?
Not really, I guess we should add the holder, then do the merge.
>
> (Maybe I'm not reading the code right).
>
> > @@ -1302,8 +1309,10 @@ static void process_deferred_bios(struct pool *pool)
> > return;
> > }
> >
> > - while ((bio = bio_list_pop(&bios)))
> > + while ((bio = bio_list_pop(&bios))) {
> > + BUG_ON(bio->bi_next);
> > generic_make_request(bio);
> > + }
>
> bio_list_pop() will always: bio->bi_next = NULL;
>
> so there is no need for the BUG_ON() here.
ok.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Don't use the bi_next field for the holder of a cell.
