lkml.org 
[lkml]   [2010]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] dm: Compute average flush time from component devices
    On Mon, Nov 29 2010 at  5:05pm -0500,
    Darrick J. Wong <djwong@us.ibm.com> wrote:

    > For dm devices which are composed of other block devices, a flush is mapped out
    > to those other block devices. Therefore, the average flush time can be
    > computed as the average flush time of whichever device flushes most slowly.

    I share Neil's concern about having to track such fine grained
    additional state in order to make the FS behave somewhat better. What
    are the _real_ fsync-happy workloads which warrant this optimization?

    That concern aside, my comments on your proposed DM changes are inlined below.

    > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
    > index 7cb1352..62aeeb9 100644
    > --- a/drivers/md/dm.c
    > +++ b/drivers/md/dm.c
    > @@ -846,12 +846,38 @@ static void start_queue(struct request_queue *q)
    > spin_unlock_irqrestore(q->queue_lock, flags);
    > }
    >
    > +static void measure_flushes(struct mapped_device *md)
    > +{
    > + struct dm_table *t;
    > + struct dm_dev_internal *dd;
    > + struct list_head *devices;
    > + u64 max = 0, samples = 0;
    > +
    > + t = dm_get_live_table(md);
    > + devices = dm_table_get_devices(t);
    > + list_for_each_entry(dd, devices, list) {
    > + if (dd->dm_dev.bdev->bd_disk->avg_flush_time_ns <= max)
    > + continue;
    > + max = dd->dm_dev.bdev->bd_disk->avg_flush_time_ns;
    > + samples = dd->dm_dev.bdev->bd_disk->flush_samples;
    > + }
    > + dm_table_put(t);
    > +
    > + spin_lock(&md->disk->flush_time_lock);
    > + md->disk->avg_flush_time_ns = max;
    > + md->disk->flush_samples = samples;
    > + spin_unlock(&md->disk->flush_time_lock);
    > +}
    > +

    You're checking all devices in a table rather than all devices that will
    receive a flush. The devices that will receive a flush is left for each
    target to determine (target exposes num_flush_requests). I'd prefer to
    see a more controlled .iterate_devices() based iteration of devices in
    each target.

    dm-table.c:dm_calculate_queue_limits() shows how iterate_devices can be
    used to combine device specific data using a common callback and a data
    pointer -- for that data pointer we'd need a local temporary structure
    with your 'max' and 'samples' members.

    > static void dm_done(struct request *clone, int error, bool mapped)
    > {
    > int r = error;
    > struct dm_rq_target_io *tio = clone->end_io_data;
    > dm_request_endio_fn rq_end_io = tio->ti->type->rq_end_io;
    >
    > + if (clone->cmd_flags & REQ_FLUSH)
    > + measure_flushes(tio->md);
    > +
    > if (mapped && rq_end_io)
    > r = rq_end_io(tio->ti, clone, error, &tio->info);
    >
    > @@ -2310,6 +2336,8 @@ static void dm_wq_work(struct work_struct *work)
    > if (dm_request_based(md))
    > generic_make_request(c);
    > else
    > + if (c->bi_rw & REQ_FLUSH)
    > + measure_flushes(md);
    > __split_and_process_bio(md, c);
    >
    > down_read(&md->io_lock);
    >

    You're missing important curly braces for the else in your dm_wq_work()
    change...

    But the bio-based call to measure_flushes() (dm_wq_work's call) should
    be pushed into __split_and_process_bio() -- and maybe measure_flushes()
    could grow a 'struct dm_table *table' argument that, if not NULL, avoids
    getting the reference that __split_and_process_bio() already has on the
    live table.

    Mike


    \
     
     \ /
      Last update: 2010-11-30 06:23    [W:0.035 / U:30.320 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site