| Message ID | 20190610090730.8589-5-pagupta@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | virtio pmem driver | expand |
On Mon, Jun 10 2019 at 5:07am -0400, Pankaj Gupta <pagupta@redhat.com> wrote: > This patch sets dax device 'DAXDEV_SYNC' flag if all the target > devices of device mapper support synchrononous DAX. If device > mapper consists of both synchronous and asynchronous dax devices, > we don't set 'DAXDEV_SYNC' flag. > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > --- > drivers/md/dm-table.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 350cf0451456..c5160d846fe6 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -890,10 +890,17 @@ static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, > start, len); > } > > +static int device_synchronous(struct dm_target *ti, struct dm_dev *dev, > + sector_t start, sector_t len, void *data) > +{ > + return dax_synchronous(dev->dax_dev); > +} > + > bool dm_table_supports_dax(struct dm_table *t, int blocksize) > { > struct dm_target *ti; > unsigned i; > + bool dax_sync = true; > > /* Ensure that all targets support DAX. */ > for (i = 0; i < dm_table_get_num_targets(t); i++) { > @@ -906,7 +913,14 @@ bool dm_table_supports_dax(struct dm_table *t, int blocksize) > !ti->type->iterate_devices(ti, device_supports_dax, > &blocksize)) > return false; > + > + /* Check devices support synchronous DAX */ > + if (dax_sync && > + !ti->type->iterate_devices(ti, device_synchronous, NULL)) > + dax_sync = false; > } > + if (dax_sync) > + set_dax_synchronous(t->md->dax_dev); > > return true; > } > -- > 2.20.1 > dm_table_supports_dax() is called multiple times (from dm_table_set_restrictions and dm_table_determine_type). It is strange to have a getter have a side-effect of being a setter too. Overloading like this could get you in trouble in the future. Are you certain this is what you want? Or would it be better to refactor dm_table_supports_dax() to take an iterate_devices_fn arg and have callers pass the appropriate function? Then have dm_table_set_restrictions() caller do: if (dm_table_supports_dax(t, device_synchronous, NULL)) set_dax_synchronous(t->md->dax_dev); (NULL arg implies dm_table_supports_dax() refactoring would take a int *data pointer rather than int type). Mike
Hi Mike, Thanks for the review Please find my reply inline. > > dm_table_supports_dax() is called multiple times (from > dm_table_set_restrictions and dm_table_determine_type). It is strange > to have a getter have a side-effect of being a setter too. Overloading > like this could get you in trouble in the future. > > Are you certain this is what you want? I agree with you. > > Or would it be better to refactor dm_table_supports_dax() to take an > iterate_devices_fn arg and have callers pass the appropriate function? > Then have dm_table_set_restrictions() caller do: > > if (dm_table_supports_dax(t, device_synchronous, NULL)) > set_dax_synchronous(t->md->dax_dev); > > (NULL arg implies dm_table_supports_dax() refactoring would take a int > *data pointer rather than int type). > > Mike > I am sending below patch as per your suggestion. Does it look near to what you have in mind? Thank you, Pankaj =============== diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 350cf0451456..8d89acc8b8c2 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -881,7 +881,7 @@ void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type) EXPORT_SYMBOL_GPL(dm_table_set_type); /* validate the dax capability of the target device span */ -static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, +int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, sector_t start, sector_t len, void *data) { int blocksize = *(int *) data; @@ -890,7 +890,15 @@ static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, start, len); } -bool dm_table_supports_dax(struct dm_table *t, int blocksize) +/* Check devices support synchronous DAX */ +static int device_synchronous(struct dm_target *ti, struct dm_dev *dev, + sector_t start, sector_t len, void *data) +{ + return dax_synchronous(dev->dax_dev); +} + +bool dm_table_supports_dax(struct dm_table *t, + iterate_devices_callout_fn iterate_fn, int *blocksize) { struct dm_target *ti; unsigned i; @@ -903,8 +911,7 @@ bool dm_table_supports_dax(struct dm_table *t, int blocksize) return false; if (!ti->type->iterate_devices || - !ti->type->iterate_devices(ti, device_supports_dax, - &blocksize)) + !ti->type->iterate_devices(ti, iterate_fn, blocksize)) return false; } @@ -940,6 +947,7 @@ static int dm_table_determine_type(struct dm_table *t) struct dm_target *tgt; struct list_head *devices = dm_table_get_devices(t); enum dm_queue_mode live_md_type = dm_get_md_type(t->md); + int page_size = PAGE_SIZE; if (t->type != DM_TYPE_NONE) { /* target already set the table's type */ @@ -984,7 +992,7 @@ static int dm_table_determine_type(struct dm_table *t) verify_bio_based: /* We must use this table as bio-based */ t->type = DM_TYPE_BIO_BASED; - if (dm_table_supports_dax(t, PAGE_SIZE) || + if (dm_table_supports_dax(t, device_supports_dax, &page_size) || (list_empty(devices) && live_md_type == DM_TYPE_DAX_BIO_BASED)) { t->type = DM_TYPE_DAX_BIO_BASED; } else { @@ -1883,6 +1891,7 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, struct queue_limits *limits) { bool wc = false, fua = false; + int page_size = PAGE_SIZE; /* * Copy table's limits to the DM device's request_queue @@ -1910,8 +1919,13 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, } blk_queue_write_cache(q, wc, fua); - if (dm_table_supports_dax(t, PAGE_SIZE)) + if (dm_table_supports_dax(t, device_supports_dax, &page_size)) { + blk_queue_flag_set(QUEUE_FLAG_DAX, q); + if (dm_table_supports_dax(t, device_synchronous, NULL)) + set_dax_synchronous(t->md->dax_dev); + } else blk_queue_flag_clear(QUEUE_FLAG_DAX, q); diff --git a/drivers/md/dm.c b/drivers/md/dm.c index b1caa7188209..b92c42a72ad4 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1119,7 +1119,7 @@ static bool dm_dax_supported(struct dax_device *dax_dev, struct block_device *bd if (!map) return false; - ret = dm_table_supports_dax(map, blocksize); + ret = dm_table_supports_dax(map, device_supports_dax, &blocksize); dm_put_live_table(md, srcu_idx); diff --git a/drivers/md/dm.h b/drivers/md/dm.h index 17e3db54404c..0475673337f3 100644 --- a/drivers/md/dm.h +++ b/drivers/md/dm.h @@ -72,7 +72,10 @@ bool dm_table_bio_based(struct dm_table *t); bool dm_table_request_based(struct dm_table *t); void dm_table_free_md_mempools(struct dm_table *t); struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t); -bool dm_table_supports_dax(struct dm_table *t, int blocksize); +bool dm_table_supports_dax(struct dm_table *t, iterate_devices_callout_fn fn, + int *blocksize); +int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, + sector_t start, sector_t len, void *data); void dm_lock_md_type(struct mapped_device *md); void dm_unlock_md_type(struct mapped_device *md); -- 2.20.1
On Tue, Jun 11 2019 at 9:10am -0400, Pankaj Gupta <pagupta@redhat.com> wrote: > Hi Mike, > > Thanks for the review Please find my reply inline. > > > > > dm_table_supports_dax() is called multiple times (from > > dm_table_set_restrictions and dm_table_determine_type). It is strange > > to have a getter have a side-effect of being a setter too. Overloading > > like this could get you in trouble in the future. > > > > Are you certain this is what you want? > > I agree with you. > > > > > Or would it be better to refactor dm_table_supports_dax() to take an > > iterate_devices_fn arg and have callers pass the appropriate function? > > Then have dm_table_set_restrictions() caller do: > > > > if (dm_table_supports_dax(t, device_synchronous, NULL)) > > set_dax_synchronous(t->md->dax_dev); > > > > (NULL arg implies dm_table_supports_dax() refactoring would take a int > > *data pointer rather than int type). > > > > Mike > > > > I am sending below patch as per your suggestion. Does it look > near to what you have in mind? Yes, it does.. just one nit I noticed inlined below. > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 350cf0451456..8d89acc8b8c2 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c ... > @@ -1910,8 +1919,13 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, > } > blk_queue_write_cache(q, wc, fua); > > - if (dm_table_supports_dax(t, PAGE_SIZE)) > + if (dm_table_supports_dax(t, device_supports_dax, &page_size)) { > + No need for an empty newline here ^ > blk_queue_flag_set(QUEUE_FLAG_DAX, q); > + if (dm_table_supports_dax(t, device_synchronous, NULL)) > + set_dax_synchronous(t->md->dax_dev); > + } > else > blk_queue_flag_clear(QUEUE_FLAG_DAX, q); > Thanks, Mike
> > > Hi Mike, > > > > Thanks for the review Please find my reply inline. > > > > > > > > dm_table_supports_dax() is called multiple times (from > > > dm_table_set_restrictions and dm_table_determine_type). It is strange > > > to have a getter have a side-effect of being a setter too. Overloading > > > like this could get you in trouble in the future. > > > > > > Are you certain this is what you want? > > > > I agree with you. > > > > > > > > Or would it be better to refactor dm_table_supports_dax() to take an > > > iterate_devices_fn arg and have callers pass the appropriate function? > > > Then have dm_table_set_restrictions() caller do: > > > > > > if (dm_table_supports_dax(t, device_synchronous, NULL)) > > > set_dax_synchronous(t->md->dax_dev); > > > > > > (NULL arg implies dm_table_supports_dax() refactoring would take a int > > > *data pointer rather than int type). > > > > > > Mike > > > > > > > I am sending below patch as per your suggestion. Does it look > > near to what you have in mind? > > Yes, it does.. just one nit I noticed inlined below. > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > > index 350cf0451456..8d89acc8b8c2 100644 > > --- a/drivers/md/dm-table.c > > +++ b/drivers/md/dm-table.c > > ... > > > @@ -1910,8 +1919,13 @@ void dm_table_set_restrictions(struct dm_table *t, > > struct request_queue *q, > > } > > blk_queue_write_cache(q, wc, fua); > > > > - if (dm_table_supports_dax(t, PAGE_SIZE)) > > + if (dm_table_supports_dax(t, device_supports_dax, &page_size)) { > > + > > No need for an empty newline here ^ Sure. Will remove this and send official v12 patchset with the updated patch 4. Thanks, Pankaj > > > blk_queue_flag_set(QUEUE_FLAG_DAX, q); > > + if (dm_table_supports_dax(t, device_synchronous, NULL)) > > + set_dax_synchronous(t->md->dax_dev); > > + } > > else > > blk_queue_flag_clear(QUEUE_FLAG_DAX, q); > > > > Thanks, > Mike > >
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 350cf0451456..c5160d846fe6 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -890,10 +890,17 @@ static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, start, len); } +static int device_synchronous(struct dm_target *ti, struct dm_dev *dev, + sector_t start, sector_t len, void *data) +{ + return dax_synchronous(dev->dax_dev); +} + bool dm_table_supports_dax(struct dm_table *t, int blocksize) { struct dm_target *ti; unsigned i; + bool dax_sync = true; /* Ensure that all targets support DAX. */ for (i = 0; i < dm_table_get_num_targets(t); i++) { @@ -906,7 +913,14 @@ bool dm_table_supports_dax(struct dm_table *t, int blocksize) !ti->type->iterate_devices(ti, device_supports_dax, &blocksize)) return false; + + /* Check devices support synchronous DAX */ + if (dax_sync && + !ti->type->iterate_devices(ti, device_synchronous, NULL)) + dax_sync = false; } + if (dax_sync) + set_dax_synchronous(t->md->dax_dev); return true; }
This patch sets dax device 'DAXDEV_SYNC' flag if all the target devices of device mapper support synchrononous DAX. If device mapper consists of both synchronous and asynchronous dax devices, we don't set 'DAXDEV_SYNC' flag. Signed-off-by: Pankaj Gupta <pagupta@redhat.com> --- drivers/md/dm-table.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)