[dm-devel] [PATCH] bio-integrity: revert "stop abusing bi_end_io"
On Wed, 2 Aug 2017, Christoph Hellwig wrote:
> On Wed, Aug 02, 2017 at 02:27:50PM +0200, Milan Broz wrote:
> > In dm-integrity target we register integrity profile that have
> > both generate_fn and verify_fn callbacks set to NULL.
> >
> > This is used if dm-integrity is stacked under a dm-crypt device
> > for authenticated encryption (integrity payload contains authentication
> > tag and IV seed).
> >
> > In this case the verification is done through own crypto API
> > processing inside dm-crypt; integrity profile is only holder
> > of these data. (And memory is owned by dm-crypt as well.)
>
> Maybe that's where the problem lies? You're abusing the integrity
> payload for something that is not end to end data integrity at all
> and then wonder why it breaks? Also the commit that introduced your
> code had absolutely no review by Martin or any of the core block
> folks.
>
> The right fix is to revert the dm-crypt commit.
That dm-crypt commit that uses bio integrity payload came 3 months before
7c20f11680a441df09de7235206f70115fbf6290 and it was already present in
4.12.
The patch 7c20f11680a441df09de7235206f70115fbf6290 not only breaks
dm-crypt and dm-integrity. It also breaks regular integrity payload usage
when stacking drivers are used.
Suppose that a bio goes the through the device mapper stack. At each level
a clone bio is allocated and forwarded to a lower level. It eventually
reaches the request-based physical disk driver.
In the kernel 4.12, when the bio reaches the request-based driver, the
function blk_queue_bio is called, it calls bio_integrity_prep,
bio_integrity_prep saves the bio->bi_end_io in the structure
bio_integrity_payload and sets bio->bi_end_io = bio_integrity_endio. When
the bio is finished, bio_integrity_endio is called, it performs the
verification (a call to bio_integrity_verify_fn), restores bio->bi_end_io
and calls it.
The patch 7c20f11680a441df09de7235206f70115fbf6290 changes it so that
bio->bi_end_io is not changed and bio_integrity_endio is called directly
from bio_endio if the bio has integrity payload. The unintended
consequence of this change is that when a request with integrity payload
is finished and bio_endio is called for each cloned bio, the verification
routine bio_integrity_verify_fn is called for every level in the stack (it
used to be called only for the lowest level in 4.12). At different levels
in the stack, the bio may have different bi_sector value, so the repeated
verification can't succeed.
I think the patch 7c20f11680a441df09de7235206f70115fbf6290 should be
reverted and it should be reworked for the next merge window, so that it
calls bio_integrity_verify_fn only for the lowest level.
Mikulas
From: Mikulas Patocka
The patch 7c20f11680a441df09de7235206f70115fbf6290 ("bio-integrity: stop
abusing bi_end_io") changes the code so that it doesn't replace
bio_end_io with bio_integrity_endio and calls bio_integrity_endio directly
from bio_endio.
The unintended consequence of this change is that when a bio with
integrity payload is passed through the device stack, bio_integrity_endio
is called for each level of the stack (it used to be called only for the
lowest level before).
This breaks dm-integrity and dm-crypt and it also causes verification
errors when a bio with regular integrity payload is passed through the
device mapper stack.
Signed-off-by: Mikulas Patocka
diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index f733beab6ca2..8df4eb103ba9 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -102,7 +102,7 @@ EXPORT_SYMBOL(bio_integrity_alloc);
* Description: Used to free the integrity portion of a bio. Usually
* called from bio_free().
*/
-static void bio_integrity_free(struct bio *bio)
+void bio_integrity_free(struct bio *bio)
{
struct bio_integrity_payload *bip = bio_integrity(bio);
struct bio_set *bs = bio->bi_pool;
@@ -120,8 +120,8 @@ static void bio_integrity_free(struct bio *bio)
}
bio->bi_integrity = NULL;
- bio->bi_opf &= ~REQ_INTEGRITY;
}
+EXPORT_SYMBOL(bio_integrity_free);
/**
* bio_integrity_add_page - Attach integrity metadata
@@ -326,6 +326,12 @@ bool bio_integrity_prep(struct bio *bio)
offset = 0;
}
+ /* Install custom I/O completion handler if read verify is enabled */
+ if (bio_data_dir(bio) == READ) {
+ bip->bip_end_io = bio->bi_end_io;
+ bio->bi_end_io = bio_integrity_endio;
+ }
+
/* Auto-generate integrity metadata if this is a write */
if (bio_data_dir(bio) == WRITE) {
bio_integrity_process(bio, &bio->bi_iter,
@@ -369,12 +375,13 @@ static void bio_integrity_verify_fn(struct work_struct
*work)
bio->bi_status = BLK_STS_IOERR;
}
- bio_integrity_free(bio);
+ /* Restore original bio completion handler */
+ bio->bi_end_io = bip->bip_end_io;
bio_endio(bio);
}
Re: [dm-devel] [PATCH] multipath-tools: check sysfs path state for NVMe/NVMf
Merged. Thanks. On Fri, Jul 28, 2017 at 8:54 AM, Guan Junxiong wrote: > Hi Hannes, > > On 2017/7/28 14:12, Hannes Reinecke wrote: > > On 07/28/2017 03:28 AM, Guan Junxiong wrote: > >> The previous code of path_offline checking was only valid for SCSI > >> device. It returned PATH_UP for other devices and throwed path check- > >> ing to chekers. This patch supplements checking sysfs path state for > >> NVMe/NVMf devices. For example, if NVMe/NVMf path is reconnectting or > >> resetting, we return PATH_PENDING in order to skip current path check- > >> ing and reschedule path checking in the next tick as soon as possible. > >> > >> Signed-off-by: Guan Junxiong > >> --- > >> libmultipath/discovery.c | 45 ++ > +-- > >> 1 file changed, 35 insertions(+), 10 deletions(-) > >> > > Thanks for doing this. > > > >> > > Patch looks good. > > > > Reviewed-by: Hannes Reinecke > > > Nice to have any review. Thanks. > > > However, I do wonder if we really need a path checker for NVMf once we > > have this. After all, the sysfs attribute reflects that KATO status, > > which pretty much determines if we can send I/O _at all_. > > > > Cheers, > > > > Hannes > > IMO, I think Keep Alive feature is in controller granularity , _not_ > namespace > granularity. A namespace can be detached ( or disabled with nvmetcli tools) > from controller which has many namespaces attached, but the controller > sysfs > state attribute is still "live". Therefor, we do need further path checker > after we know controller is live. > > Refer to the following quoted from NVMe Spec 1.3: > "The Keep Alive feature is used by the host to determine that the > controller > is operational and by the controller to determine that the host is > operational. > The host and controller are operational when each is accessible and able to > issue or execute commands." > > > Regards > Guan > > > . > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: sync third-party headers with 3.13 upstream
Merged. Thanks. On Wed, Jun 21, 2017 at 5:14 PM, Bart Van Assche wrote: > On Wed, 2017-06-21 at 14:26 +0200, Xose Vazquez Perez wrote: > > Cc: Bart Van Assche > > Cc: Christophe Varoqui > > Cc: device-mapper development > > Signed-off-by: Xose Vazquez Perez > > Reviewed-by: Bart Van Assche -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 0/4] libmultipath: Fixes for NVME / NVMEoF
This set is now merged. Thanks. On Tue, Jul 18, 2017 at 9:29 AM, Martin Wilck wrote: > Current code fails to set up multipath maps for NVME devices in a > Linux target/Linux host combination. This series enables at least > basic operation. > > Patch 1/4 fixes a crash that happens if over-long WWIDs are encountered, > and > is not specific to NVME as such. Patch 2/4 drops > the broken test uevent_can_discard_by_devpath(). Patch 3/4 compensates > for the additional event processing required by 2/4. Patch 4/4 mangles > overlong "nvme.*" WWIDs to make them usable for multipath (related > discussion > in [1]; WWID_SIZE can't be simply increased because it has to match > device mapper's DM_NAME_LEN). > > Changes wrt v1: > 1/4: assure 0-termination by using strlcpy() (Bart van Assche) > 4/4: drop extra length test (Ben Marzinski) > > I kept the v1 Reviewed-by: and Acked-by tags because the changes are minor, > I hope that's ok for the reviewers. > > [1] http://lists.infradead.org/pipermail/linux-nvme/2017-July/011960.html > > Martin Wilck (4): > libmultipath: get_udev_uid: make sure pp->wwid is 0-terminated > libmultipath: drop uevent_can_discard_by_devpath > libmultipath: only listen for uevents with DEVTYPE=disk > libmultipath: fix over-long NVME WWIDs > > libmultipath/discovery.c | 85 ++ > +++--- > libmultipath/uevent.c| 27 +-- > 2 files changed, 82 insertions(+), 30 deletions(-) > > -- > 2.13.2 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] kpartx.rules: Fix syntax error in skip_kpartx code
Merged.
Thanks.
On Fri, Jun 23, 2017 at 11:57 PM, Martin Wilck wrote:
> Fixes: 22736419 "kpartx.rules: respect skip_kpartx flag"
> Signed-off-by: Martin Wilck
> ---
> kpartx/kpartx.rules | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
> index a9587917..64d550de 100644
> --- a/kpartx/kpartx.rules
> +++ b/kpartx/kpartx.rules
> @@ -37,7 +37,7 @@ ENV{ID_FS_USAGE}=="filesystem|other",
> ENV{ID_FS_LABEL_ENC}=="?*", \
> # Create dm tables for partitions
> ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
> ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
> -ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_
> FLAG1"
> +ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_
> FLAG1"
> ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
> ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
> RUN+="/sbin/kpartx -un -p -part /dev/$name"
> --
> 2.13.1
>
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic
Patch 1 to 9 are now merged. On Thu, Jun 22, 2017 at 4:59 PM, Martin Wilck wrote: > This patch set attempts to sanitize the logic used for consistently > handling > options that can be set both via the "features" string and explicit > multipath.conf > options. This is most prominently "no_path_retry" vs. "queue_if_no_path", > but also > "retain_attached_hw_handler" vs. the feature of the same name. > > The logic this patch set follows is: > - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored >(this is the case nowadays for almost all "real" storage arrays, thanks > to hwtable). > - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry > queue". > > ... likewise for "retain_attached_hw_handler". > > In the long run, we should get rid of the "features" settings duplicating > configuration options altogether; the patch set prepares this by printing > warning messages. > > The logic is implemented in the new function reconcile_features_with_ > options, > which is called from both select_features() and merge_hwe(). In > setup_map(), > we need to call select_features() after select_no_path_retry() to make > this work. > The actual feature setting for device-mapper is made in assemble_map(), the > patch set also fixes the logic there. > > The patch set documents the behavior in the man page, and adds some more > man page fixes. > > By skipping superfluous default initializations in load_config(), the > log messages for the respective config settings become more appropriate. > > The logic for setting hardware handler is also improved. Since kernel 4.3, > "retain_attached_hw_handler yes" is implicitly set by the kernel, and > setting > the hardware handler from user space is only possible in special > situations. > Acknowledge that in multipathd, and don't try to set or unset either this > feature or the hwhandler attribute itself if it's doomed to fail. > > Review and comments are highly welcome. > > Changes wrt v1: > - Suggestions from Ben Marzinski: > * Made sure "multipathd show config" still works (1/8). > * Fixed logging for default setting of "max_sectors" (1/8) > * Consistent internal treatment of mp->features (3/8, 4/8) > * Fixed whitespace error (6/8) > * Added deprecated warnings (8/8) > - Made sure the same logic is used in propsel.c and config.c by > calling the same function (3/8, 4/8) > - Added deprecated warnings (8/8) > > Changes wrt v2: > - Added Acked-by:/Reviewed-by: tags > - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke) > - 3/11: call select_retain_hwhandler before select_features > - 8/11: don't suggest using retain_attached_hw_handler in log msg > - 9/11, 10/11, 11/11: new > > Changes wrt v3: > - Added Reviewed-by: tags (kept Ben's Ack in 4/11 although patch slightly > changed) > - 4/11: use a buffer on stack rather than malloc (Hannes Reinecke) > - 10/11: Simplify by checking dh_state only in select_handler (Hannes > Reinecke) > > Martin Wilck (11): > libmultipath: load_config: skip setting unnecessary defaults > libmultipath: add/remove_feature: use const char* for feature > libmultipath: clarify option conflicts for "features" > libmultipath: merge_hwe: fix queue_if_no_path logic > libmultipath: assemble_map: fix queue_if_no_path logic > multipath.conf.5: document no_path_retry vs. queue_if_no_path > multipath.conf.5: Remove ??? and other minor fixes > libmultipath: add deprecated warning for some features settings > libmultipath: retain_attached_hw_handler obsolete with 4.3+ > libmultipath: don't try to set hwhandler if it is retained > libmultipath: don't [un]set queue_if_no_path after domap > > libmultipath/config.c | 31 +++- > libmultipath/configure.c | 28 --- > libmultipath/dict.c| 11 +++-- > libmultipath/dmparser.c| 8 +-- > libmultipath/propsel.c | 121 ++ > +-- > libmultipath/propsel.h | 3 ++ > libmultipath/structs.c | 30 +-- > libmultipath/structs.h | 4 +- > libmultipath/util.c| 36 ++ > libmultipath/util.h| 2 + > multipath/multipath.conf.5 | 67 +++-- > 11 files changed, 231 insertions(+), 110 deletions(-) > > -- > 2.13.1 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: delete libdmmp/docs/man directory in make clean
Merged. Thanks. On Sat, Jun 24, 2017 at 8:06 PM, Xose Vazquez Perez wrote: > CC: Gris Ge > Cc: Christophe Varoqui > Cc: device-mapper development > Signed-off-by: Xose Vazquez Perez > --- > libdmmp/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libdmmp/Makefile b/libdmmp/Makefile > index bffc573..cdd26ed 100644 > --- a/libdmmp/Makefile > +++ b/libdmmp/Makefile > @@ -57,7 +57,7 @@ uninstall: > > clean: > $(RM) core *.a *.o *.gz *.so *.so.* > - $(RM) docs/man/*.3.gz > + $(RM) -r docs/man > $(MAKE) -C test clean > > check: all > -- > 2.13.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: move up TEMPLATE in hwtable
Merged.
Thanks.
On Fri, Jun 23, 2017 at 11:13 PM, Xose Vazquez Perez wrote:
> and the 'MD Series' comment to the right place.
>
> Cc: Christophe Varoqui
> Cc: device-mapper development
> Signed-off-by: Xose Vazquez Perez
> ---
> libmultipath/hwtable.c | 98 +-
>
> 1 file changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> index 390d143..a77da7e 100644
> --- a/libmultipath/hwtable.c
> +++ b/libmultipath/hwtable.c
> @@ -26,6 +26,55 @@
> * Moreover, if a device needs a special treatment by the SCSI
> * subsystem it should be included in drivers/scsi/scsi_devinfo.c
> */
> +
> +#if 0
> + /*
> +* Copy this TEMPLATE to add new hardware.
> +*
> +* Keep only mandatory(.vendor and .product) and modified
> attributes.
> +* Attributes with default values must be removed.
> +* .vendor, .product, .revision and .bl_product are POSIX Extended
> regex.
> +*
> +* COMPANY_NAME
> +*
> +* Maintainer : XXX
> +* Mail : XXX
> +*/
> + {
> + /* If product-ID is different from marketing name add a
> comment */
> + .vendor= "VENDOR",
> + .product = "PRODUCT",
> + .revision = "REVISION",
> + .bl_product= "BL_PRODUCT",
> + .pgpolicy = FAILOVER,
> + .uid_attribute = "ID_SERIAL",
> + .selector = "service-time 0",
> + .checker_name = TUR,
> + .alias_prefix = "mpath",
> + .features = "0",
> + .hwhandler = "0",
> + .prio_name = PRIO_CONST,
> + .prio_args = "",
> + .pgfailback= -FAILBACK_MANUAL,
> + .rr_weight = RR_WEIGHT_NONE,
> + .no_path_retry = NO_PATH_RETRY_UNDEF,
> + .minio = 1000,
> + .minio_rq = 1,
> + .flush_on_last_del = FLUSH_DISABLED,
> + .user_friendly_names = USER_FRIENDLY_NAMES_OFF,
> + .fast_io_fail = 5,
> + .dev_loss = 600,
> + .retain_hwhandler = RETAIN_HWHANDLER_ON,
> + .detect_prio = DETECT_PRIO_ON,
> + .detect_checker = DETECT_CHECKER_ON,
> + .deferred_remove = DEFERRED_REMOVE_OFF,
> + .delay_watch_checks = DELAY_CHECKS_OFF,
> + .delay_wait_checks = DELAY_CHECKS_OFF,
> + .skip_kpartx = SKIP_KPARTX_OFF,
> + .max_sectors_kb = MAX_SECTORS_KB_UNDEF,
> + },
> +#endif
> +
> static struct hwentry default_hw[] = {
> /*
> * Apple
> @@ -224,8 +273,8 @@ static struct hwentry default_hw[] = {
> .pgpolicy = MULTIBUS,
> .no_path_retry = NO_PATH_RETRY_QUEUE,
> },
> - /* MD Series */
> {
> + /* MD Series */
> .vendor= "DELL",
> .product = "^MD3",
> .bl_product= "Universal Xport",
> @@ -1059,53 +1108,6 @@ static struct hwentry default_hw[] = {
> .checker_name = DIRECTIO,
> .retain_hwhandler = RETAIN_HWHANDLER_OFF,
> },
> -#if 0
> - /*
> -* Copy this TEMPLATE to add new hardware.
> -*
> -* Keep only mandatory(.vendor and .product) and modified
> attributes.
> -* Attributes with default values must be removed.
> -* .vendor, .product, .revision and .bl_product are POSIX Extended
> regex.
> -*
> -* COMPANY_NAME
> -*
> -* Maintainer : XXX
> -* Mail : XXX
> -*/
> - {
> - /* If product-ID is different from marketing name add a
> comment */
> - .vendor= "VENDOR",
> - .product = "PRODUCT",
> - .revision = "REVISION",
> - .bl_product= "BL_PRODUCT",
> - .pgpolicy = FAILOVER,
> - .uid_attribute = "ID_SERIAL",
> - .selector = "service-time 0",
> - .checker_name = TUR,
> - .alias_prefix = "mpath",
> - .features = "0",
> - .hwhandler = "0",
> - .prio_name = PRIO_CONST,
> - .prio_args = "",
> - .pgfailback= -FAILBACK_MANUAL,
> - .rr_weight = RR_WEIGHT_NONE,
> - .no_path_retry = NO_PATH_RETRY_UNDEF,
> - .minio = 1000,
> - .minio_rq = 1,
> - .flush_on_last_del = FLUSH_DISABLED,
> - .user_friendly_names = USER_FRIENDLY_NAMES_OFF,
> - .fast_io_fail = 5,
> - .dev_loss = 600,
> - .retain_hwhandler = RETAIN_HWHANDLER_ON,
> -
