lkml.org 
[lkml]   [2011]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: blk_throtl_exit taking q->queue_lock is problematic
    On Thu, Feb 17, 2011 at 9:40 PM, NeilBrown <neilb@suse.de> wrote:
    > On Thu, 17 Feb 2011 11:59:06 -0500 Vivek Goyal <vgoyal@redhat.com> wrote:
    >> So if we do this change for performance reasons, it still makes sense
    >> but doing this change because md provided a q->queue_lock and took away that
    >> lock without notifying block layer hence we do this change, is still not
    >> the right reason, IMHO.
    >
    > Well...I like that patch, as it makes my life easier....
    >
    > But I agree that md is doing something wrong.  Now that ->queue_lock is
    > always initialised, it is wrong to leave it in a state where it not defined.
    >
    > So maybe I'll apply this (after testing it a bit.  The only reason for taking
    > the lock queue_lock in a couple of places is to silence some warnings.
    >
    > Thanks,
    > NeilBrown
    >
    >
    > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
    > index a23ffa3..909282d 100644
    > --- a/drivers/md/raid1.c
    > +++ b/drivers/md/raid1.c
    > @@ -959,7 +961,9 @@ static int make_request(mddev_t *mddev, struct bio * bio)
    >                atomic_inc(&r1_bio->remaining);
    >                spin_lock_irqsave(&conf->device_lock, flags);
    >                bio_list_add(&conf->pending_bio_list, mbio);
    > +               spin_lock(mddev->queue->queue_lock);
    >                blk_plug_device(mddev->queue);
    > +               spin_unlock(mddev->queue->queue_lock);
    >                spin_unlock_irqrestore(&conf->device_lock, flags);
    >        }
    >        r1_bio_write_done(r1_bio, bio->bi_vcnt, behind_pages, behind_pages != NULL);

    Noticed an inconsistency, raid10.c's additional locking also protects
    the bio_list_add() whereas raid1.c's doesn't. Seems the additional
    protection in raid10 isn't needed?

    > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
    > index 3b607b2..60e6cb1 100644
    > --- a/drivers/md/raid10.c
    > +++ b/drivers/md/raid10.c
    > @@ -970,8 +972,10 @@ static int make_request(mddev_t *mddev, struct bio * bio)
    >
    >                atomic_inc(&r10_bio->remaining);
    >                spin_lock_irqsave(&conf->device_lock, flags);
    > +               spin_lock(mddev->queue->queue_lock);
    >                bio_list_add(&conf->pending_bio_list, mbio);
    >                blk_plug_device(mddev->queue);
    > +               spin_unlock(mddev->queue->queue_lock);
    >                spin_unlock_irqrestore(&conf->device_lock, flags);
    >        }
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-02-18 04:23    [W:2.412 / U:0.964 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site