[PATCH 0/3] btrfs-progs: remove full /dev scanning

[PATCH 0/3] btrfs-progs: remove full /dev scanning

[Eric Sandeen]

btrfs fileystem show and btrfs device scan today both have
the "-d" option to scan everything under /dev.  But we also
have a mechanism to scan everything in /proc/partitions, which
should always be sufficient.

If anyone knows why we'd find something deep under /dev but
not in /proc/partitions, speak now or forever hold your peace...

Tested this by running through a matrix of -d, -m, or "" args
for show/scan, for a 2-device fs, with and without a symlinked
device, with and without a symlinked mountpoint.  All output was
identical.

Thanks,
-Eric

[PATCH 1/3] btrfs-progs: scan /proc/partitions not all of /dev with "-d"

[Eric Sandeen]

We can scan for btrfs devices in a few ways.  By default
libblkid is used for "device scan" and "filesystem show";
with the -m option only mounted filesystems are scanned,
and with -d we physically read every system device.

But there's no reason for the complexity of a descent through
/dev; /proc/partitions has every device known to the kernel, so
just use that when -d is specified.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/cmds-device.c b/cmds-device.c
index b6772b9..519681a 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -224,7 +224,7 @@ static int cmd_scan_dev(int argc, char **argv)
 			break;
 		switch (c) {
 		case 'd':
-			where = BTRFS_SCAN_DEV;
+			where = BTRFS_SCAN_PROC;
 			all = 1;
 			break;
 		default:
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index bf87bbe..0ad7e8f 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -614,7 +614,7 @@ static int cmd_show(int argc, char **argv)
 			break;
 		switch (c) {
 		case 'd':
-			where = BTRFS_SCAN_DEV;
+			where = BTRFS_SCAN_PROC;
 			break;
 		case 'm':
 			where = BTRFS_SCAN_MOUNTED;
@@ -638,7 +638,7 @@ static int cmd_show(int argc, char **argv)
 		 * right away
 		 */
 		if (type == BTRFS_ARG_BLKDEV) {
-			if (where == BTRFS_SCAN_DEV) {
+			if (where == BTRFS_SCAN_PROC) {
 				/* we need to do this because
 				 * legacy BTRFS_SCAN_DEV
 				 * provides /dev/dm-x paths
@@ -681,7 +681,7 @@ static int cmd_show(int argc, char **argv)
 		}
 	}
 
-	if (where == BTRFS_SCAN_DEV)
+	if (where == BTRFS_SCAN_PROC)
 		goto devs_only;
 
 	/* show mounted btrfs */

Re: [PATCH 1/3] btrfs-progs: scan /proc/partitions not all of /dev with "-d"

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>

On 21/08/2014 06:22, Eric Sandeen wrote:
> We can scan for btrfs devices in a few ways.  By default
> libblkid is used for "device scan" and "filesystem show";
> with the -m option only mounted filesystems are scanned,
> and with -d we physically read every system device.
>
> But there's no reason for the complexity of a descent through
> /dev; /proc/partitions has every device known to the kernel, so
> just use that when -d is specified.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> diff --git a/cmds-device.c b/cmds-device.c
> index b6772b9..519681a 100644
> --- a/cmds-device.c
> +++ b/cmds-device.c
> @@ -224,7 +224,7 @@ static int cmd_scan_dev(int argc, char **argv)
>   			break;
>   		switch (c) {
>   		case 'd':
> -			where = BTRFS_SCAN_DEV;
> +			where = BTRFS_SCAN_PROC;
>   			all = 1;
>   			break;
>   		default:
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index bf87bbe..0ad7e8f 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -614,7 +614,7 @@ static int cmd_show(int argc, char **argv)
>   			break;
>   		switch (c) {
>   		case 'd':
> -			where = BTRFS_SCAN_DEV;
> +			where = BTRFS_SCAN_PROC;
>   			break;
>   		case 'm':
>   			where = BTRFS_SCAN_MOUNTED;
> @@ -638,7 +638,7 @@ static int cmd_show(int argc, char **argv)
>   		 * right away
>   		 */
>   		if (type == BTRFS_ARG_BLKDEV) {
> -			if (where == BTRFS_SCAN_DEV) {
> +			if (where == BTRFS_SCAN_PROC) {
>   				/* we need to do this because
>   				 * legacy BTRFS_SCAN_DEV
>   				 * provides /dev/dm-x paths
> @@ -681,7 +681,7 @@ static int cmd_show(int argc, char **argv)
>   		}
>   	}
>
> -	if (where == BTRFS_SCAN_DEV)
> +	if (where == BTRFS_SCAN_PROC)
>   		goto devs_only;
>
>   	/* show mounted btrfs */
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/3] btrfs-progs: don't fall back to recursive /dev scan

[Eric Sandeen]

If we didn't find what we are looking for in /proc/partitions,
we're not going to find it by scanning every node under /dev, either.

But that's just what btrfs_scan_for_fsid() does.

Remove that fallback; at that point btrfs_scan_for_fsid() just calls
scan_for_btrfs(), so remove the wrapper & call it directly.

Side note: so, these paths always use /proc/partitions, not libblkid.
Userspace-intiated scans default to libblkid.  I presume this is
part of the design, and intentional?  Anyway, not changing it now!

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/disk-io.c b/disk-io.c
index d10d647..077eb7e 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1018,7 +1018,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	}
 
 	if (total_devs != 1) {
-		ret = btrfs_scan_for_fsid(run_ioctl);
+		ret = scan_for_btrfs(BTRFS_SCAN_PROC, run_ioctl);
 		if (ret)
 			return ret;
 	}
diff --git a/utils.c b/utils.c
index d61cbec..12ed7a2 100644
--- a/utils.c
+++ b/utils.c
@@ -1151,7 +1151,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan other devices */
 	if (is_btrfs && total_devs > 1) {
-		if ((ret = btrfs_scan_for_fsid(!BTRFS_UPDATE_KERNEL)))
+		if ((ret = scan_for_btrfs(BTRFS_SCAN_PROC, !BTRFS_UPDATE_KERNEL)))
 			return ret;
 	}
 
@@ -1339,16 +1339,6 @@ fail:
 	return ret;
 }
 
-int btrfs_scan_for_fsid(int run_ioctls)
-{
-	int ret;
-
-	ret = scan_for_btrfs(BTRFS_SCAN_PROC, run_ioctls);
-	if (ret)
-		ret = scan_for_btrfs(BTRFS_SCAN_DEV, run_ioctls);
-	return ret;
-}
-
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset)
 {

Re: [PATCH 2/3] btrfs-progs: don't fall back to recursive /dev scan

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 21/08/2014 06:23, Eric Sandeen wrote:
> If we didn't find what we are looking for in /proc/partitions,
> we're not going to find it by scanning every node under /dev, either.
>
> But that's just what btrfs_scan_for_fsid() does.
>
> Remove that fallback; at that point btrfs_scan_for_fsid() just calls
> scan_for_btrfs(), so remove the wrapper & call it directly.
>
> Side note: so, these paths always use /proc/partitions, not libblkid.
> Userspace-intiated scans default to libblkid.  I presume this is
> part of the design, and intentional?  Anyway, not changing it now!
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/disk-io.c b/disk-io.c
> index d10d647..077eb7e 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -1018,7 +1018,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
>   	}
>
>   	if (total_devs != 1) {
> -		ret = btrfs_scan_for_fsid(run_ioctl);
> +		ret = scan_for_btrfs(BTRFS_SCAN_PROC, run_ioctl);
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/utils.c b/utils.c
> index d61cbec..12ed7a2 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1151,7 +1151,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>
>   	/* scan other devices */
>   	if (is_btrfs && total_devs > 1) {
> -		if ((ret = btrfs_scan_for_fsid(!BTRFS_UPDATE_KERNEL)))
> +		if ((ret = scan_for_btrfs(BTRFS_SCAN_PROC, !BTRFS_UPDATE_KERNEL)))
>   			return ret;
>   	}
>
> @@ -1339,16 +1339,6 @@ fail:
>   	return ret;
>   }
>
> -int btrfs_scan_for_fsid(int run_ioctls)
> -{
> -	int ret;
> -
> -	ret = scan_for_btrfs(BTRFS_SCAN_PROC, run_ioctls);
> -	if (ret)
> -		ret = scan_for_btrfs(BTRFS_SCAN_DEV, run_ioctls);
> -	return ret;
> -}
> -
>   int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>   				 int super_offset)
>   {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 3/3] btrfs-progs: remove BTRFS_SCAN_DEV and btrfs_scan_one_dir

[Eric Sandeen]

After the previous 2 patches, nothing uses
whole-dev-tree scanning, so remove the code which
implemented that functionality.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/utils.c b/utils.c
index 12ed7a2..765a214 100644
--- a/utils.c
+++ b/utils.c
@@ -1228,117 +1228,6 @@ void btrfs_register_one_device(char *fname)
 	close(fd);
 }
 
-int btrfs_scan_one_dir(char *dirname, int run_ioctl)
-{
-	DIR *dirp = NULL;
-	struct dirent *dirent;
-	struct pending_dir *pending;
-	struct stat st;
-	int ret;
-	int fd;
-	int dirname_len;
-	char *fullpath;
-	struct list_head pending_list;
-	struct btrfs_fs_devices *tmp_devices;
-	u64 num_devices;
-
-	INIT_LIST_HEAD(&pending_list);
-
-	pending = malloc(sizeof(*pending));
-	if (!pending)
-		return -ENOMEM;
-	strcpy(pending->name, dirname);
-
-again:
-	dirname_len = strlen(pending->name);
-	fullpath = malloc(PATH_MAX);
-	dirname = pending->name;
-
-	if (!fullpath) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	dirp = opendir(dirname);
-	if (!dirp) {
-		fprintf(stderr, "Unable to open %s for scanning\n", dirname);
-		ret = -errno;
-		goto fail;
-	}
-	while(1) {
-		dirent = readdir(dirp);
-		if (!dirent)
-			break;
-		if (dirent->d_name[0] == '.')
-			continue;
-		if (dirname_len + strlen(dirent->d_name) + 2 > PATH_MAX) {
-			ret = -EFAULT;
-			goto fail;
-		}
-		snprintf(fullpath, PATH_MAX, "%s/%s", dirname, dirent->d_name);
-		ret = lstat(fullpath, &st);
-		if (ret < 0) {
-			fprintf(stderr, "failed to stat %s\n", fullpath);
-			continue;
-		}
-		if (S_ISLNK(st.st_mode))
-			continue;
-		if (S_ISDIR(st.st_mode)) {
-			struct pending_dir *next = malloc(sizeof(*next));
-			if (!next) {
-				ret = -ENOMEM;
-				goto fail;
-			}
-			strcpy(next->name, fullpath);
-			list_add_tail(&next->list, &pending_list);
-		}
-		if (!S_ISBLK(st.st_mode)) {
-			continue;
-		}
-		fd = open(fullpath, O_RDONLY);
-		if (fd < 0) {
-			/* ignore the following errors:
-				ENXIO (device don't exists) 
-				ENOMEDIUM (No medium found -> 
-					like a cd tray empty)
-			*/
-			if(errno != ENXIO && errno != ENOMEDIUM) 
-				fprintf(stderr, "failed to read %s: %s\n", 
-					fullpath, strerror(errno));
-			continue;
-		}
-		ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
-					    &num_devices,
-					    BTRFS_SUPER_INFO_OFFSET, 0);
-		if (ret == 0 && run_ioctl > 0) {
-			btrfs_register_one_device(fullpath);
-		}
-		close(fd);
-	}
-	if (!list_empty(&pending_list)) {
-		free(pending);
-		pending = list_entry(pending_list.next, struct pending_dir,
-				     list);
-		free(fullpath);
-		list_del(&pending->list);
-		closedir(dirp);
-		dirp = NULL;
-		goto again;
-	}
-	ret = 0;
-fail:
-	free(pending);
-	free(fullpath);
-	while (!list_empty(&pending_list)) {
-		pending = list_entry(pending_list.next, struct pending_dir,
-				     list);
-		list_del(&pending->list);
-		free(pending);
-	}
-	if (dirp)
-		closedir(dirp);
-	return ret;
-}
-
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset)
 {
@@ -2291,9 +2180,6 @@ int scan_for_btrfs(int where, int update_kernel)
 	case BTRFS_SCAN_PROC:
 		ret = btrfs_scan_block_devices(update_kernel);
 		break;
-	case BTRFS_SCAN_DEV:
-		ret = btrfs_scan_one_dir("/dev", update_kernel);
-		break;
 	case BTRFS_SCAN_LBLKID:
 		ret = btrfs_scan_lblkid(update_kernel);
 		break;
diff --git a/utils.h b/utils.h
index 0c9b65f..3a05131 100644
--- a/utils.h
+++ b/utils.h
@@ -27,9 +27,8 @@
 #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024)
 
 #define BTRFS_SCAN_PROC		(1ULL << 0)
-#define BTRFS_SCAN_DEV		(1ULL << 1)
-#define BTRFS_SCAN_MOUNTED	(1ULL << 2)
-#define BTRFS_SCAN_LBLKID	(1ULL << 3)
+#define BTRFS_SCAN_MOUNTED	(1ULL << 1)
+#define BTRFS_SCAN_LBLKID	(1ULL << 2)
 
 #define BTRFS_UPDATE_KERNEL	1
 
@@ -70,7 +69,6 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 		      u32 sectorsize);
 int btrfs_scan_for_fsid(int run_ioctls);
 void btrfs_register_one_device(char *fname);
-int btrfs_scan_one_dir(char *dirname, int run_ioctl);
 char *canonicalize_dm_name(const char *ptname);
 char *canonicalize_path(const char *path);
 int check_mounted(const char *devicename);

Re: [PATCH 3/3] btrfs-progs: remove BTRFS_SCAN_DEV and btrfs_scan_one_dir

[Anand Jain]

  :-( to see the entire btrfs_scan_one_dir() going away.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 21/08/2014 06:24, Eric Sandeen wrote:
> After the previous 2 patches, nothing uses
> whole-dev-tree scanning, so remove the code which
> implemented that functionality.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/utils.c b/utils.c
> index 12ed7a2..765a214 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1228,117 +1228,6 @@ void btrfs_register_one_device(char *fname)
>   	close(fd);
>   }
>
> -int btrfs_scan_one_dir(char *dirname, int run_ioctl)
> -{
> -	DIR *dirp = NULL;
> -	struct dirent *dirent;
> -	struct pending_dir *pending;
> -	struct stat st;
> -	int ret;
> -	int fd;
> -	int dirname_len;
> -	char *fullpath;
> -	struct list_head pending_list;
> -	struct btrfs_fs_devices *tmp_devices;
> -	u64 num_devices;
> -
> -	INIT_LIST_HEAD(&pending_list);
> -
> -	pending = malloc(sizeof(*pending));
> -	if (!pending)
> -		return -ENOMEM;
> -	strcpy(pending->name, dirname);
> -
> -again:
> -	dirname_len = strlen(pending->name);
> -	fullpath = malloc(PATH_MAX);
> -	dirname = pending->name;
> -
> -	if (!fullpath) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	dirp = opendir(dirname);
> -	if (!dirp) {
> -		fprintf(stderr, "Unable to open %s for scanning\n", dirname);
> -		ret = -errno;
> -		goto fail;
> -	}
> -	while(1) {
> -		dirent = readdir(dirp);
> -		if (!dirent)
> -			break;
> -		if (dirent->d_name[0] == '.')
> -			continue;
> -		if (dirname_len + strlen(dirent->d_name) + 2 > PATH_MAX) {
> -			ret = -EFAULT;
> -			goto fail;
> -		}
> -		snprintf(fullpath, PATH_MAX, "%s/%s", dirname, dirent->d_name);
> -		ret = lstat(fullpath, &st);
> -		if (ret < 0) {
> -			fprintf(stderr, "failed to stat %s\n", fullpath);
> -			continue;
> -		}
> -		if (S_ISLNK(st.st_mode))
> -			continue;
> -		if (S_ISDIR(st.st_mode)) {
> -			struct pending_dir *next = malloc(sizeof(*next));
> -			if (!next) {
> -				ret = -ENOMEM;
> -				goto fail;
> -			}
> -			strcpy(next->name, fullpath);
> -			list_add_tail(&next->list, &pending_list);
> -		}
> -		if (!S_ISBLK(st.st_mode)) {
> -			continue;
> -		}
> -		fd = open(fullpath, O_RDONLY);
> -		if (fd < 0) {
> -			/* ignore the following errors:
> -				ENXIO (device don't exists)
> -				ENOMEDIUM (No medium found ->
> -					like a cd tray empty)
> -			*/
> -			if(errno != ENXIO && errno != ENOMEDIUM)
> -				fprintf(stderr, "failed to read %s: %s\n",
> -					fullpath, strerror(errno));
> -			continue;
> -		}
> -		ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
> -					    &num_devices,
> -					    BTRFS_SUPER_INFO_OFFSET, 0);
> -		if (ret == 0 && run_ioctl > 0) {
> -			btrfs_register_one_device(fullpath);
> -		}
> -		close(fd);
> -	}
> -	if (!list_empty(&pending_list)) {
> -		free(pending);
> -		pending = list_entry(pending_list.next, struct pending_dir,
> -				     list);
> -		free(fullpath);
> -		list_del(&pending->list);
> -		closedir(dirp);
> -		dirp = NULL;
> -		goto again;
> -	}
> -	ret = 0;
> -fail:
> -	free(pending);
> -	free(fullpath);
> -	while (!list_empty(&pending_list)) {
> -		pending = list_entry(pending_list.next, struct pending_dir,
> -				     list);
> -		list_del(&pending->list);
> -		free(pending);
> -	}
> -	if (dirp)
> -		closedir(dirp);
> -	return ret;
> -}
> -
>   int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
>   				 int super_offset)
>   {
> @@ -2291,9 +2180,6 @@ int scan_for_btrfs(int where, int update_kernel)
>   	case BTRFS_SCAN_PROC:
>   		ret = btrfs_scan_block_devices(update_kernel);
>   		break;
> -	case BTRFS_SCAN_DEV:
> -		ret = btrfs_scan_one_dir("/dev", update_kernel);
> -		break;
>   	case BTRFS_SCAN_LBLKID:
>   		ret = btrfs_scan_lblkid(update_kernel);
>   		break;
> diff --git a/utils.h b/utils.h
> index 0c9b65f..3a05131 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -27,9 +27,8 @@
>   #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024)
>
>   #define BTRFS_SCAN_PROC		(1ULL << 0)
> -#define BTRFS_SCAN_DEV		(1ULL << 1)
> -#define BTRFS_SCAN_MOUNTED	(1ULL << 2)
> -#define BTRFS_SCAN_LBLKID	(1ULL << 3)
> +#define BTRFS_SCAN_MOUNTED	(1ULL << 1)
> +#define BTRFS_SCAN_LBLKID	(1ULL << 2)
>
>   #define BTRFS_UPDATE_KERNEL	1
>
> @@ -70,7 +69,6 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
>   		      u32 sectorsize);
>   int btrfs_scan_for_fsid(int run_ioctls);
>   void btrfs_register_one_device(char *fname);
> -int btrfs_scan_one_dir(char *dirname, int run_ioctl);
>   char *canonicalize_dm_name(const char *ptname);
>   char *canonicalize_path(const char *path);
>   int check_mounted(const char *devicename);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Anand Jain]

  A long time back there was an attempt to remove it but
  this avoided it. Pls ref to the link in this discussion.

  https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg27272.html

Thanks, Anand

On 08/21/2014 06:21 AM, Eric Sandeen wrote:
> btrfs fileystem show and btrfs device scan today both have
> the "-d" option to scan everything under /dev.  But we also
> have a mechanism to scan everything in /proc/partitions, which
> should always be sufficient.
>
> If anyone knows why we'd find something deep under /dev but
> not in /proc/partitions, speak now or forever hold your peace...
>
> Tested this by running through a matrix of -d, -m, or "" args
> for show/scan, for a 2-device fs, with and without a symlinked
> device, with and without a symlinked mountpoint.  All output was
> identical.
>
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Eric Sandeen]

On 8/21/14, 3:44 AM, Anand Jain wrote:
> 
> 
>  A long time back there was an attempt to remove it but
>  this avoided it. Pls ref to the link in this discussion.
> 
>  https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg27272.html

Hm, I guess I don't understand this.  How is udev related to whether
or not /proc/partitions is sufficient vs. recursive /dev?

To be clear, my patchset keeps the -d / --all-devices option.

It simply discovers all devices via /proc/partitions, not via
a full /dev tree walk.

Thanks,
-Eric


> Thanks, Anand
> 
> On 08/21/2014 06:21 AM, Eric Sandeen wrote:
>> btrfs fileystem show and btrfs device scan today both have
>> the "-d" option to scan everything under /dev.  But we also
>> have a mechanism to scan everything in /proc/partitions, which
>> should always be sufficient.
>>
>> If anyone knows why we'd find something deep under /dev but
>> not in /proc/partitions, speak now or forever hold your peace...
>>
>> Tested this by running through a matrix of -d, -m, or "" args
>> for show/scan, for a 2-device fs, with and without a symlinked
>> device, with and without a symlinked mountpoint.  All output was
>> identical.
>>
>> Thanks,
>> -Eric
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Anand Jain]

On 21/08/2014 22:29, Eric Sandeen wrote:
> On 8/21/14, 3:44 AM, Anand Jain wrote:
>>
>>
>>   A long time back there was an attempt to remove it but
>>   this avoided it. Pls ref to the link in this discussion.
>>
>>   https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg27272.html
>
> Hm, I guess I don't understand this.  How is udev related to whether
> or not /proc/partitions is sufficient vs. recursive /dev?
>
> To be clear, my patchset keeps the -d / --all-devices option.
>
> It simply discovers all devices via /proc/partitions, not via
> a full /dev tree walk.

  Makes sense.

Thanks, Anand


> Thanks,
> -Eric
>
>
>> Thanks, Anand
>>
>> On 08/21/2014 06:21 AM, Eric Sandeen wrote:
>>> btrfs fileystem show and btrfs device scan today both have
>>> the "-d" option to scan everything under /dev.  But we also
>>> have a mechanism to scan everything in /proc/partitions, which
>>> should always be sufficient.
>>>
>>> If anyone knows why we'd find something deep under /dev but
>>> not in /proc/partitions, speak now or forever hold your peace...
>>>
>>> Tested this by running through a matrix of -d, -m, or "" args
>>> for show/scan, for a 2-device fs, with and without a symlinked
>>> device, with and without a symlinked mountpoint.  All output was
>>> identical.
>>>
>>> Thanks,
>>> -Eric
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[David Sterba]

On Thu, Aug 21, 2014 at 09:29:56AM -0500, Eric Sandeen wrote:
> On 8/21/14, 3:44 AM, Anand Jain wrote:
> > 
> > 
> >  A long time back there was an attempt to remove it but
> >  this avoided it. Pls ref to the link in this discussion.
> > 
> >  https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg27272.html
> 
> Hm, I guess I don't understand this.  How is udev related to whether
> or not /proc/partitions is sufficient vs. recursive /dev?

The argument about udev is probably invalid, /proc/filesystem is
populated by kernel afaics.

> To be clear, my patchset keeps the -d / --all-devices option.
> 
> It simply discovers all devices via /proc/partitions, not via
> a full /dev tree walk.

I'm fine with that.

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Goffredo Baroncelli]

On 08/21/2014 12:21 AM, Eric Sandeen wrote:
> btrfs fileystem show and btrfs device scan today both have
> the "-d" option to scan everything under /dev.  But we also
> have a mechanism to scan everything in /proc/partitions, which
> should always be sufficient.
> 
> If anyone knows why we'd find something deep under /dev but
> not in /proc/partitions, speak now or forever hold your peace...

In the past I worked on this. From my commit messages [1]

[...]
the devices "scanned" are extracted from /proc/partitions. This
should avoid to scan devices not suitable for a btrfs filesystem like cdrom
and floppy or to scan not existant devices.
[...]

Of course I (we) don't care cdrom and/or floppy, but this raises 
the question: are there other block devices which aren't showed in 
proc/partitions ? 
I am thinking to some less common hardware like
NON USB sd disk (I saw this kind of hardware, but now I don't have 
it in my hands....).

GB

[1] 0dbd99fb3e117cd5f87eda492b6b4fab1b5bea23
> 
> Tested this by running through a matrix of -d, -m, or "" args
> for show/scan, for a 2-device fs, with and without a symlinked
> device, with and without a symlinked mountpoint.  All output was
> identical.
> 
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[David Sterba]

On Thu, Aug 21, 2014 at 08:08:57PM +0200, Goffredo Baroncelli wrote:
> Of course I (we) don't care cdrom and/or floppy, but this raises 
> the question: are there other block devices which aren't showed in 
> proc/partitions ? 

cdrom appears as /dev/sr0, floppy is /dev/fd0 if the respective modules
are loaded, no breakage here.

> I am thinking to some less common hardware like
> NON USB sd disk (I saw this kind of hardware, but now I don't have 
> it in my hands....).

A block device whose driver calls register_blkdev should appear in
proc/partitions, if not then I think it's a bug or a very non-standard
interface.

I don't know about any other cases where full /dev scan would reveal
something that /proc/partitions not, so I guess it's safe to remove it.

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Eric Sandeen]

On 8/26/14, 5:55 AM, David Sterba wrote:
> On Thu, Aug 21, 2014 at 08:08:57PM +0200, Goffredo Baroncelli wrote:
>> Of course I (we) don't care cdrom and/or floppy, but this raises 
>> the question: are there other block devices which aren't showed in 
>> proc/partitions ? 
> 
> cdrom appears as /dev/sr0, floppy is /dev/fd0 if the respective modules
> are loaded, no breakage here.
> 
>> I am thinking to some less common hardware like
>> NON USB sd disk (I saw this kind of hardware, but now I don't have 
>> it in my hands....).
> 
> A block device whose driver calls register_blkdev should appear in
> proc/partitions, if not then I think it's a bug or a very non-standard
> interface.
> 
> I don't know about any other cases where full /dev scan would reveal
> something that /proc/partitions not, so I guess it's safe to remove it.

Yesterday, Chris did mention <something something mumble> about /dev/mapper/*

[12:57]  <cmason> I got inconsistent results from /proc/partitions
[12:57]  <cmason> this was 3 years ago now
[12:58]  <cmason> it didn't find everything for dm devices, yeah
[12:58]  <cmason> especially dm multipath got really confused

and I'll be honest, I didn't test with devicemapper devices, much
less dm multipath, so I suppose that's warranted prior to removal.

I'll see what I can do.

If there's a problem, I bet there's a solution that doesn't involve
scanning everything under /dev ...

-Eric

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Chris Mason]

On 08/26/2014 10:51 AM, Eric Sandeen wrote:
> On 8/26/14, 5:55 AM, David Sterba wrote:
>> On Thu, Aug 21, 2014 at 08:08:57PM +0200, Goffredo Baroncelli wrote:
>>> Of course I (we) don't care cdrom and/or floppy, but this raises 
>>> the question: are there other block devices which aren't showed in 
>>> proc/partitions ? 
>>
>> cdrom appears as /dev/sr0, floppy is /dev/fd0 if the respective modules
>> are loaded, no breakage here.
>>
>>> I am thinking to some less common hardware like
>>> NON USB sd disk (I saw this kind of hardware, but now I don't have 
>>> it in my hands....).
>>
>> A block device whose driver calls register_blkdev should appear in
>> proc/partitions, if not then I think it's a bug or a very non-standard
>> interface.
>>
>> I don't know about any other cases where full /dev scan would reveal
>> something that /proc/partitions not, so I guess it's safe to remove it.
> 
> Yesterday, Chris did mention <something something mumble> about /dev/mapper/*
> 
> [12:57]  <cmason> I got inconsistent results from /proc/partitions
> [12:57]  <cmason> this was 3 years ago now
> [12:58]  <cmason> it didn't find everything for dm devices, yeah
> [12:58]  <cmason> especially dm multipath got really confused
> 
> and I'll be honest, I didn't test with devicemapper devices, much
> less dm multipath, so I suppose that's warranted prior to removal.
> 
> I'll see what I can do.
> 
> If there's a problem, I bet there's a solution that doesn't involve
> scanning everything under /dev ...

That's definitely the problem I hit, but it was years ago.  I'm willing
to bet we're safe to keep /proc/partitions now.

-chris

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Anand Jain]

On 26/08/2014 22:53, Chris Mason wrote:
> On 08/26/2014 10:51 AM, Eric Sandeen wrote:
>> On 8/26/14, 5:55 AM, David Sterba wrote:
>>> On Thu, Aug 21, 2014 at 08:08:57PM +0200, Goffredo Baroncelli wrote:
>>>> Of course I (we) don't care cdrom and/or floppy, but this raises
>>>> the question: are there other block devices which aren't showed in
>>>> proc/partitions ?
>>>
>>> cdrom appears as /dev/sr0, floppy is /dev/fd0 if the respective modules
>>> are loaded, no breakage here.
>>>
>>>> I am thinking to some less common hardware like
>>>> NON USB sd disk (I saw this kind of hardware, but now I don't have
>>>> it in my hands....).
>>>
>>> A block device whose driver calls register_blkdev should appear in
>>> proc/partitions, if not then I think it's a bug or a very non-standard
>>> interface.
>>>
>>> I don't know about any other cases where full /dev scan would reveal
>>> something that /proc/partitions not, so I guess it's safe to remove it.
>>
>> Yesterday, Chris did mention <something something mumble> about /dev/mapper/*
>>
>> [12:57]  <cmason> I got inconsistent results from /proc/partitions
>> [12:57]  <cmason> this was 3 years ago now
>> [12:58]  <cmason> it didn't find everything for dm devices, yeah
>> [12:58]  <cmason> especially dm multipath got really confused
>>
>> and I'll be honest, I didn't test with devicemapper devices, much
>> less dm multipath, so I suppose that's warranted prior to removal.
>>
>> I'll see what I can do.
>>
>> If there's a problem, I bet there's a solution that doesn't involve
>> scanning everything under /dev ...
>
> That's definitely the problem I hit, but it was years ago.  I'm willing
> to bet we're safe to keep /proc/partitions now.

  I was tempted to remove BTRFS_SCAN_PROC completely when I introduced
  BTRFS_SCAN_LBLKID, but then there was less clarity on this as it is
  now.

  The point I am trying to make is that we could remove BTRFS_SCAN_PROC
  all together and keep only the BTRFS_SCAN_LBLKID.
  (Which means we shall remove the -d option under btrfs filesystem
  show and btrfs device scan).

  We could remove scanning /proc/partition (the BTRFS_SCAN_PROC)
  because the libblkid (under scan method BTRFS_SCAN_LBLKID) would scan
  both udev and /proc/partition by default. (as in the libblkid
  man page). Further libblkid is end user configurable. end user can
  override the default configuration and can scan either of udev
  /dev/disk/by-* symlinks or /proc/partitions only.

  Further if still some device is not found then it would be a libblkid
  bug/enhancement, so we need to fix it under libblkid.

  The scan method BTRFS_SCAN_PROC is redundant.

Thanks, Anand


> -chris
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/3] btrfs-progs: remove full /dev scanning

[Anand Jain]

  Apparently we don't need -d option at all. The libblkid does
  cover both udev and /proc/partitions.

  ----
  EVALUATE=<methods>
    Defines LABEL and UUID evaluation method(s). Currently, the libblkid 
library supports "udev" and "scan" methods. More than one methods may be 
specified in a comma separated list. Default is "udev,scan". The "udev" 
method uses udev /dev/disk/by-* symlinks and the "scan" method scans all 
block devices from the /proc/partitions file.
  ----

  We could mark -d option as deprecated (in btrfs dev scan and
  btrfs fi show) or if agreed I would prefer totally removal.
  That will clean up the code quite nicely.

Thanks, Anand


On 21/08/2014 06:21, Eric Sandeen wrote:
> btrfs fileystem show and btrfs device scan today both have
> the "-d" option to scan everything under /dev.  But we also
> have a mechanism to scan everything in /proc/partitions, which
> should always be sufficient.
>
> If anyone knows why we'd find something deep under /dev but
> not in /proc/partitions, speak now or forever hold your peace...
>
> Tested this by running through a matrix of -d, -m, or "" args
> for show/scan, for a 2-device fs, with and without a symlinked
> device, with and without a symlinked mountpoint.  All output was
> identical.
>
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>