* PR API fixes for multipathing @ 2016-07-08 12:23 Christoph Hellwig 2016-07-08 12:23 ` [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations Christoph Hellwig ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Christoph Hellwig @ 2016-07-08 12:23 UTC (permalink / raw) To: dm-devel, linux-scsi; +Cc: linux-block I was a bit overeager to thing ALL_TG_PT would solve all our multipathing woes in respect to persistent reservation. Turns out that there are lots of possible setups where it doesn't work, and we'll have to ask device mapper to register all underlying devices instead. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations 2016-07-08 12:23 PR API fixes for multipathing Christoph Hellwig @ 2016-07-08 12:23 ` Christoph Hellwig 2016-07-08 15:56 ` Mike Christie 2016-07-08 12:23 ` [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device Christoph Hellwig 2016-07-15 19:03 ` PR API fixes for multipathing Martin K. Petersen 2 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2016-07-08 12:23 UTC (permalink / raw) To: dm-devel, linux-scsi; +Cc: linux-block These only work if the we use the same initiator ID for all path, which might not be true if we use different protocols, or even just different HBAs. Instead dm-mpath will grow support to register all path manually later in this series. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/scsi/sd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 60bff78..42b4d05 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1613,8 +1613,7 @@ static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, return -EOPNOTSUPP; return sd_pr_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00, old_key, new_key, 0, - (1 << 0) /* APTPL */ | - (1 << 2) /* ALL_TG_PT */); + (1 << 0) /* APTPL */); } static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type, -- 2.1.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations 2016-07-08 12:23 ` [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations Christoph Hellwig @ 2016-07-08 15:56 ` Mike Christie 0 siblings, 0 replies; 11+ messages in thread From: Mike Christie @ 2016-07-08 15:56 UTC (permalink / raw) To: Christoph Hellwig, dm-devel, linux-scsi; +Cc: linux-block On 07/08/2016 07:23 AM, Christoph Hellwig wrote: > These only work if the we use the same initiator ID for all path, > which might not be true if we use different protocols, or even just > different HBAs. > > Instead dm-mpath will grow support to register all path manually > later in this series. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/scsi/sd.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 60bff78..42b4d05 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1613,8 +1613,7 @@ static int sd_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, > return -EOPNOTSUPP; > return sd_pr_command(bdev, (flags & PR_FL_IGNORE_KEY) ? 0x06 : 0x00, > old_key, new_key, 0, > - (1 << 0) /* APTPL */ | > - (1 << 2) /* ALL_TG_PT */); > + (1 << 0) /* APTPL */); > } > > static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type, > Reviewed-by: Mike Christie <mchristi@redhat.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device 2016-07-08 12:23 PR API fixes for multipathing Christoph Hellwig 2016-07-08 12:23 ` [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations Christoph Hellwig @ 2016-07-08 12:23 ` Christoph Hellwig 2016-07-08 15:57 ` Mike Christie 2016-07-15 19:03 ` PR API fixes for multipathing Martin K. Petersen 2 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2016-07-08 12:23 UTC (permalink / raw) To: dm-devel, linux-scsi; +Cc: linux-block So far we tried to rely on the SCSI 'all target ports' bit to register all path, but for many setups this didn't work properly as the different path aren seen as separate initiators to the target instead of multiple ports of the same initiator. Because of that we'll stop setting the 'all target ports' bit in SCSI, and let device mapper handle iterating over the device for each path and register it manually. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/dm.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 15 deletions(-) diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 1b2f962..e4e98b7 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -3601,26 +3601,81 @@ void dm_free_md_mempools(struct dm_md_mempools *pools) kfree(pools); } -static int dm_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, - u32 flags) +struct dm_pr { + u64 old_key; + u64 new_key; + u32 flags; + bool fail_early; +}; + +static int dm_call_pr(struct block_device *bdev, iterate_devices_callout_fn fn, + void *data) { struct mapped_device *md = bdev->bd_disk->private_data; - const struct pr_ops *ops; - fmode_t mode; - int r; + struct dm_table *table; + struct dm_target *ti; + int ret = 0, srcu_idx; - r = dm_grab_bdev_for_ioctl(md, &bdev, &mode); - if (r < 0) - return r; + table = dm_get_live_table(md, &srcu_idx); + if (!table || !dm_table_get_size(table)) + return -ENOTTY; - ops = bdev->bd_disk->fops->pr_ops; - if (ops && ops->pr_register) - r = ops->pr_register(bdev, old_key, new_key, flags); - else - r = -EOPNOTSUPP; + /* We only support devices that have a single target */ + ret = -ENOTTY; + if (dm_table_get_num_targets(table) != 1) + goto out; + ti = dm_table_get_target(table, 0); - bdput(bdev); - return r; + ret = -EINVAL; + if (!ti->type->iterate_devices) + goto out; + + ret = ti->type->iterate_devices(ti, fn, data); + if (ret) + goto out; + + ret = 0; +out: + dm_put_live_table(md, srcu_idx); + return ret; +} + +/* + * For register / unregister we need to manually call out to every path. + */ +static int __dm_pr_register(struct dm_target *ti, struct dm_dev *dev, + sector_t start, sector_t len, void *data) +{ + struct dm_pr *pr = data; + const struct pr_ops *ops = dev->bdev->bd_disk->fops->pr_ops; + + if (!ops || !ops->pr_register) + return -EOPNOTSUPP; + return ops->pr_register(dev->bdev, pr->old_key, pr->new_key, pr->flags); +} + +static int dm_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, + u32 flags) +{ + struct dm_pr pr = { + .old_key = old_key, + .new_key = new_key, + .flags = flags, + .fail_early = true, + }; + int ret; + + ret = dm_call_pr(bdev, __dm_pr_register, &pr); + if (ret && new_key) { + /* unregister all paths if we failed to register any path */ + pr.old_key = new_key; + pr.new_key = 0; + pr.flags = 0; + pr.fail_early = false; + dm_call_pr(bdev, __dm_pr_register, &pr); + } + + return ret; } static int dm_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type, -- 2.1.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device 2016-07-08 12:23 ` [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device Christoph Hellwig @ 2016-07-08 15:57 ` Mike Christie 0 siblings, 0 replies; 11+ messages in thread From: Mike Christie @ 2016-07-08 15:57 UTC (permalink / raw) To: Christoph Hellwig, dm-devel, linux-scsi; +Cc: linux-block On 07/08/2016 07:23 AM, Christoph Hellwig wrote: > So far we tried to rely on the SCSI 'all target ports' bit to register > all path, but for many setups this didn't work properly as the different > path aren seen as separate initiators to the target instead of multiple > ports of the same initiator. Because of that we'll stop setting the > 'all target ports' bit in SCSI, and let device mapper handle iterating > over the device for each path and register it manually. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/md/dm.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 70 insertions(+), 15 deletions(-) > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 1b2f962..e4e98b7 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -3601,26 +3601,81 @@ void dm_free_md_mempools(struct dm_md_mempools *pools) > kfree(pools); > } > > -static int dm_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, > - u32 flags) > +struct dm_pr { > + u64 old_key; > + u64 new_key; > + u32 flags; > + bool fail_early; > +}; > + > +static int dm_call_pr(struct block_device *bdev, iterate_devices_callout_fn fn, > + void *data) > { > struct mapped_device *md = bdev->bd_disk->private_data; > - const struct pr_ops *ops; > - fmode_t mode; > - int r; > + struct dm_table *table; > + struct dm_target *ti; > + int ret = 0, srcu_idx; > > - r = dm_grab_bdev_for_ioctl(md, &bdev, &mode); > - if (r < 0) > - return r; > + table = dm_get_live_table(md, &srcu_idx); > + if (!table || !dm_table_get_size(table)) > + return -ENOTTY; > > - ops = bdev->bd_disk->fops->pr_ops; > - if (ops && ops->pr_register) > - r = ops->pr_register(bdev, old_key, new_key, flags); > - else > - r = -EOPNOTSUPP; > + /* We only support devices that have a single target */ > + ret = -ENOTTY; > + if (dm_table_get_num_targets(table) != 1) > + goto out; > + ti = dm_table_get_target(table, 0); > > - bdput(bdev); > - return r; > + ret = -EINVAL; > + if (!ti->type->iterate_devices) > + goto out; > + > + ret = ti->type->iterate_devices(ti, fn, data); > + if (ret) > + goto out; > + > + ret = 0; > +out: > + dm_put_live_table(md, srcu_idx); > + return ret; > +} > + > +/* > + * For register / unregister we need to manually call out to every path. > + */ > +static int __dm_pr_register(struct dm_target *ti, struct dm_dev *dev, > + sector_t start, sector_t len, void *data) > +{ > + struct dm_pr *pr = data; > + const struct pr_ops *ops = dev->bdev->bd_disk->fops->pr_ops; > + > + if (!ops || !ops->pr_register) > + return -EOPNOTSUPP; > + return ops->pr_register(dev->bdev, pr->old_key, pr->new_key, pr->flags); > +} > + > +static int dm_pr_register(struct block_device *bdev, u64 old_key, u64 new_key, > + u32 flags) > +{ > + struct dm_pr pr = { > + .old_key = old_key, > + .new_key = new_key, > + .flags = flags, > + .fail_early = true, > + }; > + int ret; > + > + ret = dm_call_pr(bdev, __dm_pr_register, &pr); > + if (ret && new_key) { > + /* unregister all paths if we failed to register any path */ > + pr.old_key = new_key; > + pr.new_key = 0; > + pr.flags = 0; > + pr.fail_early = false; > + dm_call_pr(bdev, __dm_pr_register, &pr); > + } > + > + return ret; > } > > static int dm_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type, > Reviewed-by: Mike Christie <mchristi@redhat.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-08 12:23 PR API fixes for multipathing Christoph Hellwig 2016-07-08 12:23 ` [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations Christoph Hellwig 2016-07-08 12:23 ` [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device Christoph Hellwig @ 2016-07-15 19:03 ` Martin K. Petersen 2016-07-16 1:08 ` Christoph Hellwig 2 siblings, 1 reply; 11+ messages in thread From: Martin K. Petersen @ 2016-07-15 19:03 UTC (permalink / raw) To: Christoph Hellwig; +Cc: dm-devel, linux-scsi, linux-block >>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes: Christoph> I was a bit overeager to thing ALL_TG_PT would solve all our Christoph> multipathing woes in respect to persistent reservation. Christoph> Turns out that there are lots of possible setups where it Christoph> doesn't work, and we'll have to ask device mapper to register Christoph> all underlying devices instead. Should I queue the sd patch or let Mike take both through the DM tree? -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-15 19:03 ` PR API fixes for multipathing Martin K. Petersen @ 2016-07-16 1:08 ` Christoph Hellwig 2016-07-16 18:10 ` Mike Snitzer 0 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2016-07-16 1:08 UTC (permalink / raw) To: Martin K. Petersen; +Cc: Christoph Hellwig, dm-devel, linux-scsi, linux-block On Fri, Jul 15, 2016 at 03:03:54PM -0400, Martin K. Petersen wrote: > >>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes: > > Christoph> I was a bit overeager to thing ALL_TG_PT would solve all our > Christoph> multipathing woes in respect to persistent reservation. > Christoph> Turns out that there are lots of possible setups where it > Christoph> doesn't work, and we'll have to ask device mapper to register > Christoph> all underlying devices instead. > > Should I queue the sd patch or let Mike take both through the DM tree? I think having both in the same tree would be very useful. I don't care which one that is. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-16 1:08 ` Christoph Hellwig @ 2016-07-16 18:10 ` Mike Snitzer 2016-07-16 18:14 ` James Bottomley 2016-07-17 1:28 ` Christoph Hellwig 0 siblings, 2 replies; 11+ messages in thread From: Mike Snitzer @ 2016-07-16 18:10 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Martin K. Petersen, linux-block, dm-devel, linux-scsi On Fri, Jul 15 2016 at 9:08pm -0400, Christoph Hellwig <hch@lst.de> wrote: > On Fri, Jul 15, 2016 at 03:03:54PM -0400, Martin K. Petersen wrote: > > >>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes: > > > > Christoph> I was a bit overeager to thing ALL_TG_PT would solve all our > > Christoph> multipathing woes in respect to persistent reservation. > > Christoph> Turns out that there are lots of possible setups where it > > Christoph> doesn't work, and we'll have to ask device mapper to register > > Christoph> all underlying devices instead. > > > > Should I queue the sd patch or let Mike take both through the DM tree? > > I think having both in the same tree would be very useful. I don't care > which one that is. I've picked both of them up. Staged for 4.8 merge and in linux-next via linux-dm.git's 'for-next'. (I added Martin's Acked-by to the sd patch header, Martin: if not OK, or if you'd prefer Reviewed-by just let me know) Christoph, I had to fix this: drivers/md/dm.c:2564:12: warning: context imbalance in 'dm_call_pr' - different lock contexts for basic block here is the incremental I folded in to the dm patch: diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 214fa03..4dca5a7 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2567,14 +2567,13 @@ static int dm_call_pr(struct block_device *bdev, iterate_devices_callout_fn fn, struct mapped_device *md = bdev->bd_disk->private_data; struct dm_table *table; struct dm_target *ti; - int ret = 0, srcu_idx; + int ret = -ENOTTY, srcu_idx; table = dm_get_live_table(md, &srcu_idx); if (!table || !dm_table_get_size(table)) - return -ENOTTY; + goto out; /* We only support devices that have a single target */ - ret = -ENOTTY; if (dm_table_get_num_targets(table) != 1) goto out; ti = dm_table_get_target(table, 0); @@ -2584,10 +2583,6 @@ static int dm_call_pr(struct block_device *bdev, iterate_devices_callout_fn fn, goto out; ret = ti->type->iterate_devices(ti, fn, data); - if (ret) - goto out; - - ret = 0; out: dm_put_live_table(md, srcu_idx); return ret; Mike ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-16 18:10 ` Mike Snitzer @ 2016-07-16 18:14 ` James Bottomley 2016-07-16 18:25 ` Mike Snitzer 2016-07-17 1:28 ` Christoph Hellwig 1 sibling, 1 reply; 11+ messages in thread From: James Bottomley @ 2016-07-16 18:14 UTC (permalink / raw) To: Mike Snitzer, Christoph Hellwig Cc: Martin K. Petersen, linux-block, dm-devel, linux-scsi On Sat, 2016-07-16 at 14:10 -0400, Mike Snitzer wrote: > On Fri, Jul 15 2016 at 9:08pm -0400, > Christoph Hellwig <hch@lst.de> wrote: > > > On Fri, Jul 15, 2016 at 03:03:54PM -0400, Martin K. Petersen wrote: > > > > > > > > "Christoph" == Christoph Hellwig <hch@lst.de> writes: > > > > > > Christoph> I was a bit overeager to thing ALL_TG_PT would solve > > > all our > > > Christoph> multipathing woes in respect to persistent > > > reservation. > > > Christoph> Turns out that there are lots of possible setups where > > > it > > > Christoph> doesn't work, and we'll have to ask device mapper to > > > register > > > Christoph> all underlying devices instead. > > > > > > Should I queue the sd patch or let Mike take both through the DM > > > tree? > > > > I think having both in the same tree would be very useful. I don't > > care which one that is. > > I've picked both of them up. Staged for 4.8 merge and in linux-next > via linux-dm.git's 'for-next'. > > (I added Martin's Acked-by to the sd patch header, Martin: if not OK, > or if you'd prefer Reviewed-by just let me know) You are actually missing the reviewed by tags from Mike Christie on this, which should be added. James ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-16 18:14 ` James Bottomley @ 2016-07-16 18:25 ` Mike Snitzer 0 siblings, 0 replies; 11+ messages in thread From: Mike Snitzer @ 2016-07-16 18:25 UTC (permalink / raw) To: James Bottomley Cc: Christoph Hellwig, Martin K. Petersen, linux-block, dm-devel, linux-scsi On Sat, Jul 16 2016 at 2:14pm -0400, James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > On Sat, 2016-07-16 at 14:10 -0400, Mike Snitzer wrote: > > On Fri, Jul 15 2016 at 9:08pm -0400, > > Christoph Hellwig <hch@lst.de> wrote: > > > > > On Fri, Jul 15, 2016 at 03:03:54PM -0400, Martin K. Petersen wrote: > > > > > > > > > "Christoph" == Christoph Hellwig <hch@lst.de> writes: > > > > > > > > Christoph> I was a bit overeager to thing ALL_TG_PT would solve > > > > all our > > > > Christoph> multipathing woes in respect to persistent > > > > reservation. > > > > Christoph> Turns out that there are lots of possible setups where > > > > it > > > > Christoph> doesn't work, and we'll have to ask device mapper to > > > > register > > > > Christoph> all underlying devices instead. > > > > > > > > Should I queue the sd patch or let Mike take both through the DM > > > > tree? > > > > > > I think having both in the same tree would be very useful. I don't > > > care which one that is. > > > > I've picked both of them up. Staged for 4.8 merge and in linux-next > > via linux-dm.git's 'for-next'. > > > > (I added Martin's Acked-by to the sd patch header, Martin: if not OK, > > or if you'd prefer Reviewed-by just let me know) > > You are actually missing the reviewed by tags from Mike Christie on > this, which should be added. Added, thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PR API fixes for multipathing 2016-07-16 18:10 ` Mike Snitzer 2016-07-16 18:14 ` James Bottomley @ 2016-07-17 1:28 ` Christoph Hellwig 1 sibling, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2016-07-17 1:28 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, Martin K. Petersen, linux-block, dm-devel, linux-scsi On Sat, Jul 16, 2016 at 02:10:07PM -0400, Mike Snitzer wrote: > Christoph, I had to fix this: > > drivers/md/dm.c:2564:12: warning: context imbalance in 'dm_call_pr' - different lock contexts for basic block That's coming from sparse? Guess I need to update my sparse version.. > here is the incremental I folded in to the dm patch: Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-07-17 1:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-08 12:23 PR API fixes for multipathing Christoph Hellwig 2016-07-08 12:23 ` [PATCH 1/2] sd: don't use the ALL_TG_PT bit for reservations Christoph Hellwig 2016-07-08 15:56 ` Mike Christie 2016-07-08 12:23 ` [PATCH 2/2] dm: call PR reserve/unreserve on each underlying device Christoph Hellwig 2016-07-08 15:57 ` Mike Christie 2016-07-15 19:03 ` PR API fixes for multipathing Martin K. Petersen 2016-07-16 1:08 ` Christoph Hellwig 2016-07-16 18:10 ` Mike Snitzer 2016-07-16 18:14 ` James Bottomley 2016-07-16 18:25 ` Mike Snitzer 2016-07-17 1:28 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.