FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 02-07-2012, 10:18 PM
Mike Snitzer
 
Default 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.

> @@ -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].

> +
> + } 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.

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.

> @@ -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?

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?

(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.

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
 
Old 02-10-2012, 01:55 PM
Joe Thornber
 
Default 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
 

Thread Tools




All times are GMT. The time now is 05:29 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org