On 11:17, NeilBrown wrote:
> So we create a new function 'flush_pending_writes' to give that attention,
> and call it in freeze_array to be sure that we aren't waiting on raid1d.
Two minor remarks:
> +static int flush_pending_writes(conf_t *conf)
> +{
> + /* Any writes that have been queued but are awaiting
> + * bitmap updates get flushed here.
> + * We return 1 if any requests were actually submitted.
> + */
> + int rv = 0;
> +
> + spin_lock_irq(&conf->device_lock);
>
> + if (conf->pending_bio_list.head) {
> + struct bio *bio;
> + bio = bio_list_get(&conf->pending_bio_list);
> + blk_remove_plug(conf->mddev->queue);
> + spin_unlock_irq(&conf->device_lock);
> + /* flush any pending bitmap writes to disk
> + * before proceeding w/ I/O */
> + bitmap_unplug(conf->mddev->bitmap);
> +
> + while (bio) { /* submit pending writes */
> + struct bio *next = bio->bi_next;
> + bio->bi_next = NULL;
> + generic_make_request(bio);
> + bio = next;
> + }
> + rv = 1;
> + } else
> + spin_unlock_irq(&conf->device_lock);
> + return rv;
> +}
Do we really need to take the spin lock in the common case where
conf->pending_bio_list.head is NULL? If not, the above could be
optimized to the slightly faster and better readable
struct bio *bio;
if (!conf->pending_bio_list.head)
return 0;
spin_lock_irq(&conf->device_lock);
bio = bio_list_get(&conf->pending_bio_list);
...
spin_unlock_irq(&conf->device_lock);
return 1;
> diff .prev/drivers/md/raid1.c ./drivers/md/raid1.c
> --- .prev/drivers/md/raid1.c 2008-02-22 15:45:35.000000000 +1100
> +++ ./drivers/md/raid1.c 2008-02-22 15:45:35.000000000 +1100
> @@ -592,6 +592,37 @@ static int raid1_congested(void *data, i
> }
>
>
> +static int flush_pending_writes(conf_t *conf)
> +{
[snip]
Any chance to avoid this code duplication?
Regards
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe
Attachment:
signature.asc
Description: Digital signature
