lkml.org 
[lkml]   [2012]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] block: Avoid deadlocks with bio allocation by stacking drivers
    Hello, Kent.

    On Sun, Sep 09, 2012 at 05:28:10PM -0700, Kent Overstreet wrote:
    > > > + while ((bio = bio_list_pop(current->bio_list)))
    > > > + bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
    > > > +
    > > > + *current->bio_list = nopunt;
    > >
    > > Why this is necessary needs explanation and it's done in rather
    > > unusual way. I suppose the weirdness is from bio_list API
    > > restriction?
    >
    > It's because bio_lists are singly linked, so deleting an entry from the
    > middle of the list would be a real pain - just much cleaner/simpler to
    > do it this way.

    Yeah, I wonder how benefical that singly linked list is. Eh well...

    > > Wouldn't the following be better?
    > >
    > > p = mempool_alloc(bs->bi_pool, gfp_mask);
    > > if (unlikely(!p) && gfp_mask != saved_gfp) {
    > > punt_bios_to_rescuer(bs);
    > > p = mempool_alloc(bs->bi_pool, saved_gfp);
    > > }
    >
    > That'd require duplicating the error handling in two different places -
    > once for the initial allocation, once for the bvec allocation. And I
    > really hate that writing code that does
    >
    > alloc_something()
    > if (fail) {
    > alloc_something_again()
    > }
    >
    > it just screams ugly to me.

    I don't know. That at least represents what's going on and goto'ing
    back and forth is hardly pretty. Sometimes the code gets much uglier
    / unwieldy and we have to live with gotos. Here, that doesn't seem to
    be the case.

    > +static void punt_bios_to_rescuer(struct bio_set *bs)
    > +{
    > + struct bio_list punt, nopunt;
    > + struct bio *bio;
    > +
    > + /*
    > + * Don't want to punt all bios on current->bio_list; if there was a bio
    > + * on there for a stacking driver higher up in the stack, processing it
    > + * could require allocating bios from this bio_set, and we don't want to
    > + * do that from our own rescuer.

    Hmmm... isn't it more like we "must" process only the bios which are
    from this bio_set to have any kind of forward-progress guarantee? The
    above sounds like it's just something undesirable.

    Thanks.

    --
    tejun


    \
     
     \ /
      Last update: 2012-09-10 20:01    [W:0.024 / U:2.440 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site