[dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

Add -P, partition scanning option to kpartx which would set
LO_FLAGS_PARTSCAN flag during loop device creation.
This option is same as losetup -P option.

Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com>
---
 kpartx/kpartx.c | 17 +++++++++++++----
 kpartx/lopart.c |  5 ++++-
 kpartx/lopart.h |  2 +-
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 3c49999..a26d0a2 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -87,7 +87,7 @@ initpts(void)
 	addpts("ps3", read_ps3_pt);
 }
 
-static char short_opts[] = "rladfgvp:t:snu";
+static char short_opts[] = "rladfPgvp:t:snu";
 
 /* Used in gpt.c */
 int force_gpt=0;
@@ -98,7 +98,7 @@ static int
 usage(void) {
 	printf(VERSION_STRING);
 	printf("Usage:\n");
-	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] wholedisk\n");
+	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] [-P] wholedisk\n");
 	printf("\t-a add partition devmappings\n");
 	printf("\t-r devmappings will be readonly\n");
 	printf("\t-d del partition devmappings\n");
@@ -110,6 +110,7 @@ usage(void) {
 	printf("\t-v verbose\n");
 	printf("\t-n nosync mode. Return before the partitions are created\n");
 	printf("\t-s sync mode (Default). Don't return until the partitions are created\n");
+	printf("\t-P create partitioned loop device\n");
 	return 1;
 }
 
@@ -228,7 +229,7 @@ xmalloc (size_t size) {
 
 int
 main(int argc, char **argv){
-	int i, j, m, n, op, off, arg, c, d, ro=0;
+	int i, j, m, n, op, off, arg, c, d, ro=0, partscan = 0;
 	int fd = -1;
 	struct slice all;
 	struct pt *ptp;
@@ -311,6 +312,9 @@ main(int argc, char **argv){
 		case 'u':
 			what = UPDATE;
 			break;
+		case 'P':
+			partscan = 1;
+			break;
 		default:
 			usage();
 			exit(1);
@@ -328,6 +332,11 @@ main(int argc, char **argv){
 		exit(1);
 	}
 
+	if (what != ADD && partscan == 1)       {
+		printf("-P option is allowed only during loop device setup\n");
+		exit(1);
+	}
+
 	if (hotplug) {
 		/* already got [disk]device */
 	} else if (optind == argc-2) {
@@ -359,7 +368,7 @@ main(int argc, char **argv){
 			exit (0);
 
 		if (!loopdev) {
-			if (set_loop(&loopdev, rpath, 0, &ro)) {
+			if (set_loop(&loopdev, rpath, 0, &ro, partscan)) {
 				fprintf(stderr, "can't set up loop\n");
 				exit (1);
 			}
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 512a59f..9a1ce48 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -233,7 +233,8 @@ no_loop_fd:
 	return NULL;
 }
 
-int set_loop(char **device, const char *file, int offset, int *loopro)
+int set_loop(char **device, const char *file, int offset, int *loopro,
+	     int partscan)
 {
 	struct loop_info loopinfo;
 	int fd = -1, ret = 1, ffd, mode;
@@ -264,6 +265,8 @@ int set_loop(char **device, const char *file, int offset, int *loopro)
 	loopinfo.lo_offset = offset;
 	loopinfo.lo_encrypt_type = LO_CRYPT_NONE;
 	loopinfo.lo_encrypt_key_size = 0;
+	if (partscan == 1)
+		loopinfo.lo_flags |= LO_FLAGS_PARTSCAN;
 
 	if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
 		perror ("ioctl: LOOP_SET_FD");
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index c73ab23..81534e2 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,4 +1,4 @@
 extern int verbose;
-extern int set_loop (char **, const char *, int, int *);
+extern int set_loop (char **, const char *, int, int *, int);
 extern int del_loop (const char *);
 extern char * find_loop_by_file (const char *);
-- 
2.27.0

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

Hi,

Gentle ping
Please help review this.

--                
Thanks,
Ritika


On 2/11/22, 3:51 PM, Ritika Srivastava wrote:

    Add -P, partition scanning option to kpartx which would set
    LO_FLAGS_PARTSCAN flag during loop device creation.
    This option is same as losetup -P option.

    Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com>
    ---
     kpartx/kpartx.c | 17 +++++++++++++----
     kpartx/lopart.c |  5 ++++-
     kpartx/lopart.h |  2 +-
     3 files changed, 18 insertions(+), 6 deletions(-)

    diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
    index 3c49999..a26d0a2 100644
    --- a/kpartx/kpartx.c
    +++ b/kpartx/kpartx.c
    @@ -87,7 +87,7 @@ initpts(void)
     	addpts("ps3", read_ps3_pt);
     }

    -static char short_opts[] = "rladfgvp:t:snu";
    +static char short_opts[] = "rladfPgvp:t:snu";

     /* Used in gpt.c */
     int force_gpt=0;
    @@ -98,7 +98,7 @@ static int
     usage(void) {
     	printf(VERSION_STRING);
     	printf("Usage:\n");
    -	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] wholedisk\n");
    +	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] [-P] wholedisk\n");
     	printf("\t-a add partition devmappings\n");
     	printf("\t-r devmappings will be readonly\n");
     	printf("\t-d del partition devmappings\n");
    @@ -110,6 +110,7 @@ usage(void) {
     	printf("\t-v verbose\n");
     	printf("\t-n nosync mode. Return before the partitions are created\n");
     	printf("\t-s sync mode (Default). Don't return until the partitions are created\n");
    +	printf("\t-P create partitioned loop device\n");
     	return 1;
     }

    @@ -228,7 +229,7 @@ xmalloc (size_t size) {

     int
     main(int argc, char **argv){
    -	int i, j, m, n, op, off, arg, c, d, ro=0;
    +	int i, j, m, n, op, off, arg, c, d, ro=0, partscan = 0;
     	int fd = -1;
     	struct slice all;
     	struct pt *ptp;
    @@ -311,6 +312,9 @@ main(int argc, char **argv){
     		case 'u':
     			what = UPDATE;
     			break;
    +		case 'P':
    +			partscan = 1;
    +			break;
     		default:
     			usage();
     			exit(1);
    @@ -328,6 +332,11 @@ main(int argc, char **argv){
     		exit(1);
     	}

    +	if (what != ADD && partscan == 1)       {
    +		printf("-P option is allowed only during loop device setup\n");
    +		exit(1);
    +	}
    +
     	if (hotplug) {
     		/* already got [disk]device */
     	} else if (optind == argc-2) {
    @@ -359,7 +368,7 @@ main(int argc, char **argv){
     			exit (0);

     		if (!loopdev) {
    -			if (set_loop(&loopdev, rpath, 0, &ro)) {
    +			if (set_loop(&loopdev, rpath, 0, &ro, partscan)) {
     				fprintf(stderr, "can't set up loop\n");
     				exit (1);
     			}
    diff --git a/kpartx/lopart.c b/kpartx/lopart.c
    index 512a59f..9a1ce48 100644
    --- a/kpartx/lopart.c
    +++ b/kpartx/lopart.c
    @@ -233,7 +233,8 @@ no_loop_fd:
     	return NULL;
     }

    -int set_loop(char **device, const char *file, int offset, int *loopro)
    +int set_loop(char **device, const char *file, int offset, int *loopro,
    +	     int partscan)
     {
     	struct loop_info loopinfo;
     	int fd = -1, ret = 1, ffd, mode;
    @@ -264,6 +265,8 @@ int set_loop(char **device, const char *file, int offset, int *loopro)
     	loopinfo.lo_offset = offset;
     	loopinfo.lo_encrypt_type = LO_CRYPT_NONE;
     	loopinfo.lo_encrypt_key_size = 0;
    +	if (partscan == 1)
    +		loopinfo.lo_flags |= LO_FLAGS_PARTSCAN;

     	if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
     		perror ("ioctl: LOOP_SET_FD");
    diff --git a/kpartx/lopart.h b/kpartx/lopart.h
    index c73ab23..81534e2 100644
    --- a/kpartx/lopart.h
    +++ b/kpartx/lopart.h
    @@ -1,4 +1,4 @@
     extern int verbose;
    -extern int set_loop (char **, const char *, int, int *);
    +extern int set_loop (char **, const char *, int, int *, int);
     extern int del_loop (const char *);
     extern char * find_loop_by_file (const char *);
    -- 
    2.27.0

    --
    dm-devel mailing list
    dm-devel@redhat.com
    https://listman.redhat.com/mailman/listinfo/dm-devel



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Benjamin Marzinski]

On Fri, Feb 11, 2022 at 12:41:48PM -0800, Ritika Srivastava wrote:
> Add -P, partition scanning option to kpartx which would set
> LO_FLAGS_PARTSCAN flag during loop device creation.
> This option is same as losetup -P option.

I'm confused here. What's the benefit of doing this? Doesn't this create
partition devices, just like kpartx would?

-Ben

> 
> Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com>
> ---
>  kpartx/kpartx.c | 17 +++++++++++++----
>  kpartx/lopart.c |  5 ++++-
>  kpartx/lopart.h |  2 +-
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 3c49999..a26d0a2 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -87,7 +87,7 @@ initpts(void)
>  	addpts("ps3", read_ps3_pt);
>  }
>  
> -static char short_opts[] = "rladfgvp:t:snu";
> +static char short_opts[] = "rladfPgvp:t:snu";
>  
>  /* Used in gpt.c */
>  int force_gpt=0;
> @@ -98,7 +98,7 @@ static int
>  usage(void) {
>  	printf(VERSION_STRING);
>  	printf("Usage:\n");
> -	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] wholedisk\n");
> +	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] [-P] wholedisk\n");
>  	printf("\t-a add partition devmappings\n");
>  	printf("\t-r devmappings will be readonly\n");
>  	printf("\t-d del partition devmappings\n");
> @@ -110,6 +110,7 @@ usage(void) {
>  	printf("\t-v verbose\n");
>  	printf("\t-n nosync mode. Return before the partitions are created\n");
>  	printf("\t-s sync mode (Default). Don't return until the partitions are created\n");
> +	printf("\t-P create partitioned loop device\n");
>  	return 1;
>  }
>  
> @@ -228,7 +229,7 @@ xmalloc (size_t size) {
>  
>  int
>  main(int argc, char **argv){
> -	int i, j, m, n, op, off, arg, c, d, ro=0;
> +	int i, j, m, n, op, off, arg, c, d, ro=0, partscan = 0;
>  	int fd = -1;
>  	struct slice all;
>  	struct pt *ptp;
> @@ -311,6 +312,9 @@ main(int argc, char **argv){
>  		case 'u':
>  			what = UPDATE;
>  			break;
> +		case 'P':
> +			partscan = 1;
> +			break;
>  		default:
>  			usage();
>  			exit(1);
> @@ -328,6 +332,11 @@ main(int argc, char **argv){
>  		exit(1);
>  	}
>  
> +	if (what != ADD && partscan == 1)       {
> +		printf("-P option is allowed only during loop device setup\n");
> +		exit(1);
> +	}
> +
>  	if (hotplug) {
>  		/* already got [disk]device */
>  	} else if (optind == argc-2) {
> @@ -359,7 +368,7 @@ main(int argc, char **argv){
>  			exit (0);
>  
>  		if (!loopdev) {
> -			if (set_loop(&loopdev, rpath, 0, &ro)) {
> +			if (set_loop(&loopdev, rpath, 0, &ro, partscan)) {
>  				fprintf(stderr, "can't set up loop\n");
>  				exit (1);
>  			}
> diff --git a/kpartx/lopart.c b/kpartx/lopart.c
> index 512a59f..9a1ce48 100644
> --- a/kpartx/lopart.c
> +++ b/kpartx/lopart.c
> @@ -233,7 +233,8 @@ no_loop_fd:
>  	return NULL;
>  }
>  
> -int set_loop(char **device, const char *file, int offset, int *loopro)
> +int set_loop(char **device, const char *file, int offset, int *loopro,
> +	     int partscan)
>  {
>  	struct loop_info loopinfo;
>  	int fd = -1, ret = 1, ffd, mode;
> @@ -264,6 +265,8 @@ int set_loop(char **device, const char *file, int offset, int *loopro)
>  	loopinfo.lo_offset = offset;
>  	loopinfo.lo_encrypt_type = LO_CRYPT_NONE;
>  	loopinfo.lo_encrypt_key_size = 0;
> +	if (partscan == 1)
> +		loopinfo.lo_flags |= LO_FLAGS_PARTSCAN;
>  
>  	if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
>  		perror ("ioctl: LOOP_SET_FD");
> diff --git a/kpartx/lopart.h b/kpartx/lopart.h
> index c73ab23..81534e2 100644
> --- a/kpartx/lopart.h
> +++ b/kpartx/lopart.h
> @@ -1,4 +1,4 @@
>  extern int verbose;
> -extern int set_loop (char **, const char *, int, int *);
> +extern int set_loop (char **, const char *, int, int *, int);
>  extern int del_loop (const char *);
>  extern char * find_loop_by_file (const char *);
> -- 
> 2.27.0
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

On 2/22/22, 10:28 AM, "Benjamin Marzinski" wrote:

    On Fri, Feb 11, 2022 at 12:41:48PM -0800, Ritika Srivastava wrote:
    >> Add -P, partition scanning option to kpartx which would set
    >> LO_FLAGS_PARTSCAN flag during loop device creation.
    >> This option is same as losetup -P option.

    >I'm confused here. What's the benefit of doing this? Doesn't this create
    >partition devices, just like kpartx would?

This enables partition scanning for loop devices created via kpartx.
When enabled, LO_FLAGS_PARTSCAN flag will be set during loop creation via kpartx -a.
On detach (kpartx -d), any partitions (/dev/loopXpY) would also be removed.
It has similar use cases as losetup -P option.

Currently, kpartx does not create partition loop devices (/dev/loopXpY)  via kpartx -a

Example:
1. Current behavior: kpartx -a does not create /dev/loop0p1

# kpartx -av test.img
add map loop0p1 (252:0): 0 99937 linear /dev/loop0 64

(Notice below: /dev/loop0p1 NOT created)
# ls -l /dev/loop0*
brw-rw----. 1 root disk  7,   0 Jan 14 20:46 /dev/loop0 

2. IF -P IS USED to create loop device, /dev/loop0p1 IS created.

# kpartx -av -P test.img
add map loop0p1 (252:0): 0 99937 linear /dev/loop0 64

(Notice below: /dev/loop0p1 IS created)
# ls -l /dev/loop0*
brw-rw----. 1 root disk   7,   0 Jan 14 20:50 /dev/loop0
brw-rw----. 1 root disk 259,   0 Jan 14 20:50 /dev/loop0p1


Similar behavior can also be observed during detach
1. Current Behavior:  kpartx -d does NOT delete partitions

# kpartx -a -v test.img
# parted -a none -s /dev/loop0 mkpart primary 64s 100000s

# ls -l /dev/loop0*
brw-rw----. 1 root disk   7,   0 Jan 14 20:54 /dev/loop0
brw-rw----. 1 root disk 259,   0 Jan 14 20:54 /dev/loop0p1

# kpartx -d test.img
loop deleted : /dev/loop0

(Notice: /dev/loop0p1 NOT deleted)
# ls -l /dev/loop0*
brw-rw----. 1 root disk   7,   0 Jan 14 20:54 /dev/loop0
brw-rw----. 1 root disk 259,   0 Jan 14 20:54 /dev/loop0p1

2. IF -P IS added while creating loop, kpartx -d DOES delete loop0p1

# kpartx -a -v -P test.img
# parted -a none -s /dev/loop0 mkpart primary 64s 100000s

# ls -l /dev/loop0*
brw-rw----. 1 root disk   7,   0 Jan 14 20:46 /dev/loop0
brw-rw----. 1 root disk 259,   0 Jan 14 20:46 /dev/loop0p1

# kpartx -d test.img
loop deleted : /dev/loop0

(Notice: /dev/loop0p1 IS deleted)
# ls -l /dev/loop0*
brw-rw----. 1 root disk  7,   0 Jan 14 20:46 /dev/loop0

Thanks,
Ritika




--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Benjamin Marzinski]

On Tue, Feb 22, 2022 at 06:59:10PM +0000, Ritika Srivastava wrote:
> On 2/22/22, 10:28 AM, "Benjamin Marzinski" wrote:
> 
>     On Fri, Feb 11, 2022 at 12:41:48PM -0800, Ritika Srivastava wrote:
>     >> Add -P, partition scanning option to kpartx which would set
>     >> LO_FLAGS_PARTSCAN flag during loop device creation.
>     >> This option is same as losetup -P option.
> 
>     >I'm confused here. What's the benefit of doing this? Doesn't this create
>     >partition devices, just like kpartx would?
> 
> This enables partition scanning for loop devices created via kpartx.
> When enabled, LO_FLAGS_PARTSCAN flag will be set during loop creation via kpartx -a.
> On detach (kpartx -d), any partitions (/dev/loopXpY) would also be removed.
> It has similar use cases as losetup -P option.
> 
> Currently, kpartx does not create partition loop devices (/dev/loopXpY)  via kpartx -a

And I'm trying to figure out why you want to create /dev/loop0p1 if you
already have /dev/mapper/loop0p1?  The ability of kpartx to create
partitioned loop devices predates the existance "kpartx -P". So perhaps
the better question is, why do you want to run

# kpartx -av test.img

instead of just running

# loasetup -fP test.img

They both get you partition devices, and you don't need both of them.
Or do you, for some reason I don't understand?

-Ben

> Example:
> 1. Current behavior: kpartx -a does not create /dev/loop0p1
> 
> # kpartx -av test.img
> add map loop0p1 (252:0): 0 99937 linear /dev/loop0 64
> 
> (Notice below: /dev/loop0p1 NOT created)
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk  7,   0 Jan 14 20:46 /dev/loop0 
> 
> 2. IF -P IS USED to create loop device, /dev/loop0p1 IS created.
> 
> # kpartx -av -P test.img
> add map loop0p1 (252:0): 0 99937 linear /dev/loop0 64
> 
> (Notice below: /dev/loop0p1 IS created)
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk   7,   0 Jan 14 20:50 /dev/loop0
> brw-rw----. 1 root disk 259,   0 Jan 14 20:50 /dev/loop0p1
> 
> 
> Similar behavior can also be observed during detach
> 1. Current Behavior:  kpartx -d does NOT delete partitions
> 
> # kpartx -a -v test.img
> # parted -a none -s /dev/loop0 mkpart primary 64s 100000s
> 
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk   7,   0 Jan 14 20:54 /dev/loop0
> brw-rw----. 1 root disk 259,   0 Jan 14 20:54 /dev/loop0p1
> 
> # kpartx -d test.img
> loop deleted : /dev/loop0
> 
> (Notice: /dev/loop0p1 NOT deleted)
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk   7,   0 Jan 14 20:54 /dev/loop0
> brw-rw----. 1 root disk 259,   0 Jan 14 20:54 /dev/loop0p1
> 
> 2. IF -P IS added while creating loop, kpartx -d DOES delete loop0p1
> 
> # kpartx -a -v -P test.img
> # parted -a none -s /dev/loop0 mkpart primary 64s 100000s
> 
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk   7,   0 Jan 14 20:46 /dev/loop0
> brw-rw----. 1 root disk 259,   0 Jan 14 20:46 /dev/loop0p1
> 
> # kpartx -d test.img
> loop deleted : /dev/loop0
> 
> (Notice: /dev/loop0p1 IS deleted)
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk  7,   0 Jan 14 20:46 /dev/loop0
> 
> Thanks,
> Ritika
> 
> 
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

Ben,

Thank you for reviewing this.

On 2/22/22, 11:32 AM, "Benjamin Marzinski" wrote:

   > And I'm trying to figure out why you want to create /dev/loop0p1 if you
   > already have /dev/mapper/loop0p1?  The ability of kpartx to create
   > partitioned loop devices predates the existance "kpartx -P". So perhaps
   > the better question is, why do you want to run

   > # kpartx -av test.img

   > instead of just running

   > # loasetup -fP test.img

   > They both get you partition devices, and you don't need both of them.
   > Or do you, for some reason I don't understand?

My previous update may not have been clear regarding the supporting use case behind this patch.
It highlighted what the -P option would add to kpartx.

The idea is to avoid having both /dev/loop0p1 and /dev/mapper/loop0p1 and support the following use case - 
After detaching via kpartx -d, /dev/loop0p1 is still present which on subsequent kpartx -a operation results in both /dev/loop0p1 and /dev/mapper/loop0p1 being present.

Yes, you are right, workaround could be to use losetup -P the first time around when creating the loop device.
Though, having this functionality within kpartx would be nice since kpartx is also used extensively.

// workaround - losetup -P
# kpartx -a -v test.img
# ls -l /dev/loop0*
brw-rw----. 1 root disk  7,   0 Feb 22 20:05 /dev/loop0

# parted -a none -s /dev/loop0 mkpart primary 64s 100000s
# parted -a none -s /dev/loop0 set 1 lvm on
# kpartx -d test.img  
# ls -l /dev/loop0*
brw-rw----. 1 root disk   7,   0 Feb 22 20:05 /dev/loop0
brw-rw----. 1 root disk 259,   0 Feb 22 20:05 /dev/loop0p1

# kpartx -av test.img
# ls -l /dev/mapper/loop0*
lrwxrwxrwx. 1 root root       7 Feb 22 20:53 loop0p1 -> ../dm-2

# pvcreate /dev/mapper/loop0p1
# pvscan
  WARNING: Not using device /dev/loop0p1 for PV <UUID>.
  WARNING: PV <UUID> prefers device /dev/mapper/loop0p1 because device is in dm subsystem.
--                
Thanks,
Ritika



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Benjamin Marzinski]

On Tue, Feb 22, 2022 at 10:16:35PM +0000, Ritika Srivastava wrote:
> Ben,
> 
> Thank you for reviewing this.
> 
> On 2/22/22, 11:32 AM, "Benjamin Marzinski" wrote:
> 
>    > And I'm trying to figure out why you want to create /dev/loop0p1 if you
>    > already have /dev/mapper/loop0p1?  The ability of kpartx to create
>    > partitioned loop devices predates the existance "kpartx -P". So perhaps
>    > the better question is, why do you want to run
> 
>    > # kpartx -av test.img
> 
>    > instead of just running
> 
>    > # loasetup -fP test.img
> 
>    > They both get you partition devices, and you don't need both of them.
>    > Or do you, for some reason I don't understand?
> 
> My previous update may not have been clear regarding the supporting use case behind this patch.
> It highlighted what the -P option would add to kpartx.
> 
> The idea is to avoid having both /dev/loop0p1 and /dev/mapper/loop0p1 and support the following use case - 
> After detaching via kpartx -d, /dev/loop0p1 is still present which on subsequent kpartx -a operation results in both /dev/loop0p1 and /dev/mapper/loop0p1 being present.
> 
> Yes, you are right, workaround could be to use losetup -P the first time around when creating the loop device.
> Though, having this functionality within kpartx would be nice since kpartx is also used extensively.

So unless I'm missing something, we'd only really want this for removing
a kpartx device, in the case where somehow you have /dev/loopXpY
partitions without the LO_FLAGS_PARTSCAN flag set on the disk. That
seems like it shouldn't happen in the first place.  Obviously, you
showed that it can with parted.  But I would argue that this is a bug in
parted.  If parted is creating partitions, it should set
LO_FLAGS_PARTSCAN so the partition nodes get cleaned up.

I suppose kpartx could check if there are partition devices for the loop
device, and if so, it could set LO_FLAGS_PARTSCAN before doing the
remove. But setting it unilaterally would just cause it to create an
extra set of devices that would only serve to confuse people (and lvm).

Also, the actual partition /dev/loopXpY will always get removed. It's
just the devnode that stays around, and that won't confuse lvm. This
isn't that odd for loop devices. The /dev/loopX devnodes will stay
around once you're done with them, regardless of whether you create the
loop device will kpartx or losetup.


> 
> // workaround - losetup -P
> # kpartx -a -v test.img
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk  7,   0 Feb 22 20:05 /dev/loop0
> 
> # parted -a none -s /dev/loop0 mkpart primary 64s 100000s
> # parted -a none -s /dev/loop0 set 1 lvm on
> # kpartx -d test.img  
> # ls -l /dev/loop0*
> brw-rw----. 1 root disk   7,   0 Feb 22 20:05 /dev/loop0
> brw-rw----. 1 root disk 259,   0 Feb 22 20:05 /dev/loop0p1
> 
> # kpartx -av test.img
> # ls -l /dev/mapper/loop0*
> lrwxrwxrwx. 1 root root       7 Feb 22 20:53 loop0p1 -> ../dm-2
> 
> # pvcreate /dev/mapper/loop0p1
> # pvscan
>   WARNING: Not using device /dev/loop0p1 for PV <UUID>.
>   WARNING: PV <UUID> prefers device /dev/mapper/loop0p1 because device is in dm subsystem.

So this example shows exactly why I don't want both /dev/loopXpY and
/dev/mapper/loopXpY. Whenever we use your -P option we can run into
this situation, right?

Martin, do you have any feelings on this.

-Ben

> Thanks,
> Ritika
> 
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

On 2/28/22, 2:44 PM, "Benjamin Marzinski" wrote:

    > So unless I'm missing something, we'd only really want this for removing
    > a kpartx device, in the case where somehow you have /dev/loopXpY
    > partitions without the LO_FLAGS_PARTSCAN flag set on the disk. That

That's correct. We only want this option so that once PARTSCAN flag is set, 
Kpartx -d can delete /dev/loopXpY.

    > seems like it shouldn't happen in the first place.  Obviously, you
    > showed that it can with parted.  But I would argue that this is a bug in
    > parted.  If parted is creating partitions, it should set
    > LO_FLAGS_PARTSCAN so the partition nodes get cleaned up.
    > I suppose kpartx could check if there are partition devices for the loop
    > device, and if so, it could set LO_FLAGS_PARTSCAN before doing the

Would removing all partition nodes (/dev/loop0pY) on kpartx -d be a better solution.?

    > remove. But setting it unilaterally would just cause it to create an
    > extra set of devices that would only serve to confuse people (and lvm).

-P should be used only when partition scan needs to be enabled - only on need basis.

    > Also, the actual partition /dev/loopXpY will always get removed. It's
    > just the devnode that stays around, and that won't confuse lvm. This

The example below shows that /dev/loop0p1 is not removed which is confusing the LVM.

    > isn't that odd for loop devices. The /dev/loopX devnodes will stay
    > around once you're done with them, regardless of whether you create the
    > loop device will kpartx or losetup.

That's correct.
But loop device when setup with losetup -P option does remove /dev/loopXpY on detach
Hoping to achieve the same functionality in kpartx.


    >> 
    >> // workaround - losetup -P
    >> # kpartx -a -v test.img                                                                             // First kpartx
    >> # ls -l /dev/loop0*
    >> brw-rw----. 1 root disk  7,   0 Feb 22 20:05 /dev/loop0
    >> 
    >> # parted -a none -s /dev/loop0 mkpart primary 64s 100000s
    >> # parted -a none -s /dev/loop0 set 1 lvm on
    >> # kpartx -d test.img  
    >> # ls -l /dev/loop0*
    >> brw-rw----. 1 root disk   7,   0 Feb 22 20:05 /dev/loop0
    >> brw-rw----. 1 root disk 259,   0 Feb 22 20:05 /dev/loop0p1
    >> 
    >> # kpartx -av test.img                                                                         //Second kpartx
    >> # ls -l /dev/mapper/loop0*
    >> lrwxrwxrwx. 1 root root       7 Feb 22 20:53 loop0p1 -> ../dm-2
    >> 
    >> # pvcreate /dev/mapper/loop0p1
    >> # pvscan
    >>   WARNING: Not using device /dev/loop0p1 for PV <UUID>
    >>   WARNING: PV <UUID> prefers device /dev/mapper/loop0p1 because device is in dm subsystem.

    > So this example shows exactly why I don't want both /dev/loopXpY and
    > /dev/mapper/loopXpY. Whenever we use your -P option we can run into
    > this situation, right?

In the above example, the first `kpartx -a` could benefit from `-P` option and would remove /dev/loop0p1 on kpartx -d.
This is to avoid having both /dev/loop0p1 and /dev/mapper/loop0p1.

The second `kpartx -a` does not need a partition scan and `-P` option should not be provided.
However, yes, you are right - if it is specified in this case, it would create both /dev/loop0p1 and /dev/mapper/loop0p1 
- which then would have to be deleted and recreated without the `-P` option.
Maybe a warning message here would help - something like `-P` option should be used only while creating new partitions. 

Thanks,
Ritika


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Benjamin Marzinski]

On Wed, Mar 02, 2022 at 12:07:11AM +0000, Ritika Srivastava wrote:
> On 2/28/22, 2:44 PM, "Benjamin Marzinski" wrote:
> 
>     > So unless I'm missing something, we'd only really want this for removing
>     > a kpartx device, in the case where somehow you have /dev/loopXpY
>     > partitions without the LO_FLAGS_PARTSCAN flag set on the disk. That
> 
> That's correct. We only want this option so that once PARTSCAN flag is set, 
> Kpartx -d can delete /dev/loopXpY.
> 
>     > seems like it shouldn't happen in the first place.  Obviously, you
>     > showed that it can with parted.  But I would argue that this is a bug in
>     > parted.  If parted is creating partitions, it should set
>     > LO_FLAGS_PARTSCAN so the partition nodes get cleaned up.
>     > I suppose kpartx could check if there are partition devices for the loop
>     > device, and if so, it could set LO_FLAGS_PARTSCAN before doing the
> 
> Would removing all partition nodes (/dev/loop0pY) on kpartx -d be a better solution.?

Like I said, if we fix this in multipath, then checking for /dev/loopXpY
devnodes, and setting LO_FLAGS_PARTSCAN before deleting the loop device
if they are present seems like a better solution.

But again, you can make a pretty good case that when parted creates
those partition devices, it should set LO_FLAGS_PARTSCAN so that their
devnodes will get cleaned up.
 
>     > remove. But setting it unilaterally would just cause it to create an
>     > extra set of devices that would only serve to confuse people (and lvm).
> 
> -P should be used only when partition scan needs to be enabled - only on need basis.
> 
>     > Also, the actual partition /dev/loopXpY will always get removed. It's
>     > just the devnode that stays around, and that won't confuse lvm. This
> 
> The example below shows that /dev/loop0p1 is not removed which is confusing the LVM.

But lvm decides what devices are duplicates by reading them, not by
looking at their name.  So, unless both the /dev/loopXpY and the
/dev/mapper/loopXpY actually are devices, lvm won't have an issue.  If
the /dev/loopXpY devnode is still around, but there's no actual device
connected to it, that lvm issue shouldn't happen.

Leaving devnodes around doesn't cause lvm issues. But adding loop
partitions can cause lvm issues.  This is why I don't like the idea of
kpartx creating them.

>     > isn't that odd for loop devices. The /dev/loopX devnodes will stay
>     > around once you're done with them, regardless of whether you create the
>     > loop device will kpartx or losetup.
> 
> That's correct.
> But loop device when setup with losetup -P option does remove /dev/loopXpY on detach
> Hoping to achieve the same functionality in kpartx.
> 
> 
>     >> 
>     >> // workaround - losetup -P
>     >> # kpartx -a -v test.img                                                                             // First kpartx
>     >> # ls -l /dev/loop0*
>     >> brw-rw----. 1 root disk  7,   0 Feb 22 20:05 /dev/loop0
>     >> 
>     >> # parted -a none -s /dev/loop0 mkpart primary 64s 100000s
>     >> # parted -a none -s /dev/loop0 set 1 lvm on
>     >> # kpartx -d test.img  
>     >> # ls -l /dev/loop0*
>     >> brw-rw----. 1 root disk   7,   0 Feb 22 20:05 /dev/loop0
>     >> brw-rw----. 1 root disk 259,   0 Feb 22 20:05 /dev/loop0p1
>     >> 
>     >> # kpartx -av test.img                                                                         //Second kpartx
>     >> # ls -l /dev/mapper/loop0*
>     >> lrwxrwxrwx. 1 root root       7 Feb 22 20:53 loop0p1 -> ../dm-2
>     >> 
>     >> # pvcreate /dev/mapper/loop0p1
>     >> # pvscan
>     >>   WARNING: Not using device /dev/loop0p1 for PV <UUID>
>     >>   WARNING: PV <UUID> prefers device /dev/mapper/loop0p1 because device is in dm subsystem.
> 
>     > So this example shows exactly why I don't want both /dev/loopXpY and
>     > /dev/mapper/loopXpY. Whenever we use your -P option we can run into
>     > this situation, right?
> 
> In the above example, the first `kpartx -a` could benefit from `-P` option and would remove /dev/loop0p1 on kpartx -d.
> This is to avoid having both /dev/loop0p1 and /dev/mapper/loop0p1.

But again, that pvcreate WARNING happens when both devices exist. Simply
having a devnode present won't cause it. And adding the '-P' option
won't top both devices from existing at the same time. That being said,
I do understand the desire to clean them up.

-Ben

> The second `kpartx -a` does not need a partition scan and `-P` option should not be provided.
> However, yes, you are right - if it is specified in this case, it would create both /dev/loop0p1 and /dev/mapper/loop0p1 
> - which then would have to be deleted and recreated without the `-P` option.
> Maybe a warning message here would help - something like `-P` option should be used only while creating new partitions. 
> 
> Thanks,
> Ritika
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Martin Wilck]

On Wed, 2022-03-02 at 12:38 -0600, Benjamin Marzinski wrote:
> On Wed, Mar 02, 2022 at 12:07:11AM +0000, Ritika Srivastava wrote:
> > On 2/28/22, 2:44 PM, "Benjamin Marzinski" wrote:
> > 
> >     > So unless I'm missing something, we'd only really want this
> > for removing
> >     > a kpartx device, in the case where somehow you have
> > /dev/loopXpY
> >     > partitions without the LO_FLAGS_PARTSCAN flag set on the
> > disk. That
> > 
> > That's correct. We only want this option so that once PARTSCAN flag
> > is set, 
> > Kpartx -d can delete /dev/loopXpY.
> > 
> >     > seems like it shouldn't happen in the first place. 
> > Obviously, you
> >     > showed that it can with parted.  But I would argue that this
> > is a bug in
> >     > parted.  If parted is creating partitions, it should set
> >     > LO_FLAGS_PARTSCAN so the partition nodes get cleaned up.
> >     > I suppose kpartx could check if there are partition devices
> > for the loop
> >     > device, and if so, it could set LO_FLAGS_PARTSCAN before
> > doing the
> > 
> > Would removing all partition nodes (/dev/loop0pY) on kpartx -d be a
> > better solution.?
> 
> Like I said, if we fix this in multipath, then checking for
> /dev/loopXpY
> devnodes, and setting LO_FLAGS_PARTSCAN before deleting the loop
> device
> if they are present seems like a better solution.
> 
> But again, you can make a pretty good case that when parted creates
> those partition devices, it should set LO_FLAGS_PARTSCAN so that
> their
> devnodes will get cleaned up.

I guess that would do no harm. But parted, too, is agnostic of how the
loop device was created, so I wouldn't call it a bug that parted
currently doesn't set this flag. Arguably, the flag should be set when
the device is created, using "losetup -P". I find it strange that
Ritika calls that a "workaround". IMO it's the one and only correct way
to set up the loop device.

> Leaving devnodes around doesn't cause lvm issues. But adding loop
> partitions can cause lvm issues.  This is why I don't like the idea
> of
> kpartx creating them.

I agree. kpartx is a tool for creating linear dm mappings that behave
roughly like partitions. And it should stay that way. We (made the
mistake to) add convenience functionality to setup loop devices when
kpartx is called with a regular file argument. That doesn't mean that
kpartx is a generic tool for handling loop devices or partition
devices. We should stick to the "do one thing, do it right" philosophy
here.

TBH, he usage "kpartx -av /some/file" just to create the loop device,
if the file has no partition table, looks like an abuse to me. I
wouldn't recommend to rely on that behavior. It might change in the
future.

Regards,
Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

On 3/3/22, 2:57 AM, "Martin Wilck" wrote:

    > I agree. kpartx is a tool for creating linear dm mappings that behave
    > roughly like partitions. And it should stay that way. We (made the
    > mistake to) add convenience functionality to setup loop devices when
    > kpartx is called with a regular file argument. That doesn't mean that
    > kpartx is a generic tool for handling loop devices or partition
    > devices. We should stick to the "do one thing, do it right" philosophy
    > here.

    > TBH, he usage "kpartx -av /some/file" just to create the loop device,
    > if the file has no partition table, looks like an abuse to me. I
    > wouldn't recommend to rely on that behavior. It might change in the
    > future.

Thank you for clarifying this.
I was under the impression that since kpartx supports loop device creation,  
it may as well support partition scanning functionality - just like losetup does.

Will keep this in mind and be wary about using kpartx -a to create loop device.
Thank you, Ben and Martin, for your review.

Thanks,
Ritika


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH] kpartx: Add -P option for partition scanning

[Ritika Srivastava]

Add -P, partition scanning option to kpartx which would set
LO_FLAGS_PARTSCAN flag during loop device creation.
This option is same as losetup -P option.

Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com>
---
 kpartx/kpartx.c | 17 +++++++++++++----
 kpartx/lopart.c |  5 ++++-
 kpartx/lopart.h |  2 +-
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 3c49999..a26d0a2 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -87,7 +87,7 @@ initpts(void)
 	addpts("ps3", read_ps3_pt);
 }
 
-static char short_opts[] = "rladfgvp:t:snu";
+static char short_opts[] = "rladfPgvp:t:snu";
 
 /* Used in gpt.c */
 int force_gpt=0;
@@ -98,7 +98,7 @@ static int
 usage(void) {
 	printf(VERSION_STRING);
 	printf("Usage:\n");
-	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] wholedisk\n");
+	printf("  kpartx [-a|-d|-u|-l] [-r] [-p] [-f] [-g] [-s|-n] [-v] [-P] wholedisk\n");
 	printf("\t-a add partition devmappings\n");
 	printf("\t-r devmappings will be readonly\n");
 	printf("\t-d del partition devmappings\n");
@@ -110,6 +110,7 @@ usage(void) {
 	printf("\t-v verbose\n");
 	printf("\t-n nosync mode. Return before the partitions are created\n");
 	printf("\t-s sync mode (Default). Don't return until the partitions are created\n");
+	printf("\t-P create partitioned loop device\n");
 	return 1;
 }
 
@@ -228,7 +229,7 @@ xmalloc (size_t size) {
 
 int
 main(int argc, char **argv){
-	int i, j, m, n, op, off, arg, c, d, ro=0;
+	int i, j, m, n, op, off, arg, c, d, ro=0, partscan = 0;
 	int fd = -1;
 	struct slice all;
 	struct pt *ptp;
@@ -311,6 +312,9 @@ main(int argc, char **argv){
 		case 'u':
 			what = UPDATE;
 			break;
+		case 'P':
+			partscan = 1;
+			break;
 		default:
 			usage();
 			exit(1);
@@ -328,6 +332,11 @@ main(int argc, char **argv){
 		exit(1);
 	}
 
+	if (what != ADD && partscan == 1)       {
+		printf("-P option is allowed only during loop device setup\n");
+		exit(1);
+	}
+
 	if (hotplug) {
 		/* already got [disk]device */
 	} else if (optind == argc-2) {
@@ -359,7 +368,7 @@ main(int argc, char **argv){
 			exit (0);
 
 		if (!loopdev) {
-			if (set_loop(&loopdev, rpath, 0, &ro)) {
+			if (set_loop(&loopdev, rpath, 0, &ro, partscan)) {
 				fprintf(stderr, "can't set up loop\n");
 				exit (1);
 			}
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 512a59f..9a1ce48 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -233,7 +233,8 @@ no_loop_fd:
 	return NULL;
 }
 
-int set_loop(char **device, const char *file, int offset, int *loopro)
+int set_loop(char **device, const char *file, int offset, int *loopro,
+	     int partscan)
 {
 	struct loop_info loopinfo;
 	int fd = -1, ret = 1, ffd, mode;
@@ -264,6 +265,8 @@ int set_loop(char **device, const char *file, int offset, int *loopro)
 	loopinfo.lo_offset = offset;
 	loopinfo.lo_encrypt_type = LO_CRYPT_NONE;
 	loopinfo.lo_encrypt_key_size = 0;
+	if (partscan == 1)
+		loopinfo.lo_flags |= LO_FLAGS_PARTSCAN;
 
 	if (ioctl(fd, LOOP_SET_FD, (void*)(uintptr_t)(ffd)) < 0) {
 		perror ("ioctl: LOOP_SET_FD");
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index c73ab23..81534e2 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,4 +1,4 @@
 extern int verbose;
-extern int set_loop (char **, const char *, int, int *);
+extern int set_loop (char **, const char *, int, int *, int);
 extern int del_loop (const char *);
 extern char * find_loop_by_file (const char *);
-- 
2.27.0

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel