Re: [PATCH v5 01/18] block: Add PR callouts for read keys and reservation
From: Mike Christie <michael.christie@oracle.com>
Date: 2023-03-28 17:19:42
Also in:
dm-devel, linux-nvme, linux-scsi, target-devel
On 3/28/23 11:36 AM, Mike Snitzer wrote:
On Fri, Mar 24 2023 at 2:17P -0400, Mike Christie [off-list ref] wrote:quoted
Add callouts for reading keys and reservations. This allows LIO to support the READ_KEYS and READ_RESERVATION commands and will allow dm-multipath to optimize it's error handling so it can check if it's getting an error because there's an existing reservation or if we need to retry different paths.Not seeing anything in DM's path selectors that adds these optimizations. Is there accompanying multipath-tools callouts to get at this data to optimize path selection (via DM table reloads)?
You can ignore that comment. The comment was meant for the dm pr callouts
and not for normal IO/path handling, and now I think I can fix in a different
way.
I originally added the comment about better dm error handling for something
like __dm_pr_register getting a failure when it did:
ret = ops->pr_register(dev->bdev, pr->old_key, pr->new_key, pr->flags);
Right now, we fail the entire operation if just one call on one path fails. With the
pr_read_keys/reservation callouts we could check if we got a failure because there
was an existing reservation vs a retryable/ignorable error.
I forgot I wrote that comment about dm in the mail and we actually don't need the
pr_read_* callouts for that type of thing now, because I ended up changing
the existing callouts to return common error codes last kernel. So I have another
patchset that I'm working on, but am still debating about some issues like:
ret = ops->pr_register(dev->bdev, pr->old_key, pr->new_key, pr->flags);
switch (ret) {
case PR_STS_RESERVATION_CONFLICT:
pr->ret = ret;
return -1;
case PR_STS_RETRY_PATH_FAILURE:
/*
* We probably want to retry like how we do for the pg_init.
*/
....
case PR_STS_PATH_FAILED:
case PR_STS_PATH_FAST_FAILED:
/*
* I'm still not sure what to do here, because if this is the last
* host then we might want to try and register the rest of the paths
* and limp on. It probably needs a user config option.
*/
....