[PATCH 0/1] virtio: disable partitions scanning for no partitions block

[PATCH 0/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Some virtio blocks definitely have no partitions and should not be scanned.

Yury Kamenev (1):
  virtio: disable partitions scanning for no partitions block

 .../admin-guide/kernel-parameters.txt         |  3 +++
 drivers/block/Kconfig                         |  7 +++++
 drivers/block/virtio_blk.c                    | 26 +++++++++++++++++++
 include/uapi/linux/virtio_blk.h               |  2 ++
 4 files changed, 38 insertions(+)

-- 
2.24.3 (Apple Git-128)

[PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Signed-off-by: Yury Kamenev <damtev@yandex-team.ru>
---
 .../admin-guide/kernel-parameters.txt         |  3 +++
 drivers/block/Kconfig                         |  7 +++++
 drivers/block/virtio_blk.c                    | 26 +++++++++++++++++++
 include/uapi/linux/virtio_blk.h               |  2 ++
 4 files changed, 38 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index bdb22006f713..941bdaf5c167 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6076,6 +6076,9 @@
 			brightness level.
 			default: 1

+	virtiopartscan
+		Enable virtio block device partition scanning omission based on VIRTIO_BLK_F_NO_PART_SCAN feature flag.
+
 	virtio_mmio.device=
 			[VMMIO] Memory mapped virtio (platform) device.

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 63056cfd4b62..69ecd3fd7037 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -399,6 +399,13 @@ config VIRTIO_BLK
 	  This is the virtual block driver for virtio.  It can be used with
           QEMU based VMMs (like KVM or Xen).  Say Y or M.

+config VIRTIO_BLK_NO_PART_SCAN
+	bool "Disable partition scanning for devices with no partitions"
+	depends on VIRTIO_BLK
+	help
+	  Disable partition scanning for devices with no partitions.
+	  Can reduce the kernel start time for tiny systems like squashfs images.
+
 config BLK_DEV_RBD
 	tristate "Rados block device (RBD)"
 	depends on INET && BLOCK
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4b49df2dfd23..479711d3791c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -692,6 +692,19 @@ static const struct blk_mq_ops virtio_mq_ops = {
 static unsigned int virtblk_queue_depth;
 module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);

+#ifndef MODULE
+#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
+static int partitions_scanning_disable __read_mostly;
+
+static int __init partitions_scanning_setup(char *__unused)
+{
+	partitions_scanning_disable = 1;
+	return 1;
+}
+__setup("nopartscan", partitions_scanning_setup);
+#endif
+#endif
+
 static int virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -790,6 +803,13 @@ static int virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->flags |= GENHD_FL_EXT_DEVT;
 	vblk->index = index;

+#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
+	if (unlikely(partitions_scanning_disable))
+		/* disable partitions scanning if it was stated in virtio features*/
+		if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PART_SCAN))
+			vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
+#endif
+
 	/* configure queue flush support */
 	virtblk_update_cache_mode(vdev);

@@ -966,6 +986,9 @@ static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
+	VIRTIO_BLK_F_NO_PART_SCAN,
+#endif
 }
 ;
 static unsigned int features[] = {
@@ -973,6 +996,9 @@ static unsigned int features[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
+	VIRTIO_BLK_F_NO_PART_SCAN,
+#endif
 };

 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9ff..9b381675342a 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_NO_PART_SCAN	16	/* Disable partition scanning */

 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
--
2.24.3 (Apple Git-128)

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Paolo Bonzini]

On 15/07/21 11:47, Yury Kamenev wrote:
> +#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
> +	if (unlikely(partitions_scanning_disable))
> +		/* disable partitions scanning if it was stated in virtio features*/
> +		if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PART_SCAN))
> +			vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
> +#endif
> +

Has this been added to the spec?  It doesn't seem like a good idea, as 
pointed out by Stefan[1], Christoph[2] and myself[3].

Paolo

[1] 
https://lore.kernel.org/linux-block/20210524145654.GA2632@lst.de/T/#m2697cb41578490aad49ed1d8fa6604bf0924b54d
[2] 
https://lore.kernel.org/linux-block/20210524145654.GA2632@lst.de/T/#mc59329fd824102f94ac2f6b29fe94a652849aca0
[3] 
https://lore.kernel.org/linux-block/20210524145654.GA2632@lst.de/T/#mee6787c4fd87790b64feccc9e77fd5f618c2c336

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10420 bytes --]

Hi Yury,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on vhost/linux-next hch-configfs/for-next linus/master v5.14-rc1 next-20210715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yury-Kamenev/virtio-disable-partitions-scanning-for-no-partitions-block/20210715-175107
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b5b35e33f22266b3905186a005992d54ae71e51b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yury-Kamenev/virtio-disable-partitions-scanning-for-no-partitions-block/20210715-175107
        git checkout b5b35e33f22266b3905186a005992d54ae71e51b
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/block/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from drivers/block/virtio_blk.c:3:
   drivers/block/virtio_blk.c: In function 'virtblk_probe':
>> drivers/block/virtio_blk.c:807:15: error: 'partitions_scanning_disable' undeclared (first use in this function)
     807 |  if (unlikely(partitions_scanning_disable))
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   drivers/block/virtio_blk.c:807:15: note: each undeclared identifier is reported only once for each function it appears in
     807 |  if (unlikely(partitions_scanning_disable))
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^


vim +/partitions_scanning_disable +807 drivers/block/virtio_blk.c

   707	
   708	static int virtblk_probe(struct virtio_device *vdev)
   709	{
   710		struct virtio_blk *vblk;
   711		struct request_queue *q;
   712		int err, index;
   713	
   714		u32 v, blk_size, max_size, sg_elems, opt_io_size;
   715		u16 min_io_size;
   716		u8 physical_block_exp, alignment_offset;
   717		unsigned int queue_depth;
   718	
   719		if (!vdev->config->get) {
   720			dev_err(&vdev->dev, "%s failure: config access disabled\n",
   721				__func__);
   722			return -EINVAL;
   723		}
   724	
   725		err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
   726				     GFP_KERNEL);
   727		if (err < 0)
   728			goto out;
   729		index = err;
   730	
   731		/* We need to know how many segments before we allocate. */
   732		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
   733					   struct virtio_blk_config, seg_max,
   734					   &sg_elems);
   735	
   736		/* We need at least one SG element, whatever they say. */
   737		if (err || !sg_elems)
   738			sg_elems = 1;
   739	
   740		/* Prevent integer overflows and honor max vq size */
   741		sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
   742	
   743		/* We need extra sg elements at head and tail. */
   744		sg_elems += 2;
   745		vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
   746		if (!vblk) {
   747			err = -ENOMEM;
   748			goto out_free_index;
   749		}
   750	
   751		/* This reference is dropped in virtblk_remove(). */
   752		refcount_set(&vblk->refs, 1);
   753		mutex_init(&vblk->vdev_mutex);
   754	
   755		vblk->vdev = vdev;
   756		vblk->sg_elems = sg_elems;
   757	
   758		INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
   759	
   760		err = init_vq(vblk);
   761		if (err)
   762			goto out_free_vblk;
   763	
   764		/* Default queue sizing is to fill the ring. */
   765		if (likely(!virtblk_queue_depth)) {
   766			queue_depth = vblk->vqs[0].vq->num_free;
   767			/* ... but without indirect descs, we use 2 descs per req */
   768			if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
   769				queue_depth /= 2;
   770		} else {
   771			queue_depth = virtblk_queue_depth;
   772		}
   773	
   774		memset(&vblk->tag_set, 0, sizeof(vblk->tag_set));
   775		vblk->tag_set.ops = &virtio_mq_ops;
   776		vblk->tag_set.queue_depth = queue_depth;
   777		vblk->tag_set.numa_node = NUMA_NO_NODE;
   778		vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
   779		vblk->tag_set.cmd_size =
   780			sizeof(struct virtblk_req) +
   781			sizeof(struct scatterlist) * sg_elems;
   782		vblk->tag_set.driver_data = vblk;
   783		vblk->tag_set.nr_hw_queues = vblk->num_vqs;
   784	
   785		err = blk_mq_alloc_tag_set(&vblk->tag_set);
   786		if (err)
   787			goto out_free_vq;
   788	
   789		vblk->disk = blk_mq_alloc_disk(&vblk->tag_set, vblk);
   790		if (IS_ERR(vblk->disk)) {
   791			err = PTR_ERR(vblk->disk);
   792			goto out_free_tags;
   793		}
   794		q = vblk->disk->queue;
   795	
   796		virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN);
   797	
   798		vblk->disk->major = major;
   799		vblk->disk->first_minor = index_to_minor(index);
   800		vblk->disk->minors = 1 << PART_BITS;
   801		vblk->disk->private_data = vblk;
   802		vblk->disk->fops = &virtblk_fops;
   803		vblk->disk->flags |= GENHD_FL_EXT_DEVT;
   804		vblk->index = index;
   805	
   806	#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
 > 807		if (unlikely(partitions_scanning_disable))
   808			/* disable partitions scanning if it was stated in virtio features*/
   809			if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PART_SCAN))
   810				vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
   811	#endif
   812	
   813		/* configure queue flush support */
   814		virtblk_update_cache_mode(vdev);
   815	
   816		/* If disk is read-only in the host, the guest should obey */
   817		if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))
   818			set_disk_ro(vblk->disk, 1);
   819	
   820		/* We can handle whatever the host told us to handle. */
   821		blk_queue_max_segments(q, vblk->sg_elems-2);
   822	
   823		/* No real sector limit. */
   824		blk_queue_max_hw_sectors(q, -1U);
   825	
   826		max_size = virtio_max_dma_size(vdev);
   827	
   828		/* Host can optionally specify maximum segment size and number of
   829		 * segments. */
   830		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
   831					   struct virtio_blk_config, size_max, &v);
   832		if (!err)
   833			max_size = min(max_size, v);
   834	
   835		blk_queue_max_segment_size(q, max_size);
   836	
   837		/* Host can optionally specify the block size of the device */
   838		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
   839					   struct virtio_blk_config, blk_size,
   840					   &blk_size);
   841		if (!err)
   842			blk_queue_logical_block_size(q, blk_size);
   843		else
   844			blk_size = queue_logical_block_size(q);
   845	
   846		/* Use topology information if available */
   847		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
   848					   struct virtio_blk_config, physical_block_exp,
   849					   &physical_block_exp);
   850		if (!err && physical_block_exp)
   851			blk_queue_physical_block_size(q,
   852					blk_size * (1 << physical_block_exp));
   853	
   854		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
   855					   struct virtio_blk_config, alignment_offset,
   856					   &alignment_offset);
   857		if (!err && alignment_offset)
   858			blk_queue_alignment_offset(q, blk_size * alignment_offset);
   859	
   860		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
   861					   struct virtio_blk_config, min_io_size,
   862					   &min_io_size);
   863		if (!err && min_io_size)
   864			blk_queue_io_min(q, blk_size * min_io_size);
   865	
   866		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
   867					   struct virtio_blk_config, opt_io_size,
   868					   &opt_io_size);
   869		if (!err && opt_io_size)
   870			blk_queue_io_opt(q, blk_size * opt_io_size);
   871	
   872		if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
   873			q->limits.discard_granularity = blk_size;
   874	
   875			virtio_cread(vdev, struct virtio_blk_config,
   876				     discard_sector_alignment, &v);
   877			q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;
   878	
   879			virtio_cread(vdev, struct virtio_blk_config,
   880				     max_discard_sectors, &v);
   881			blk_queue_max_discard_sectors(q, v ? v : UINT_MAX);
   882	
   883			virtio_cread(vdev, struct virtio_blk_config, max_discard_seg,
   884				     &v);
   885			blk_queue_max_discard_segments(q,
   886						       min_not_zero(v,
   887								    MAX_DISCARD_SEGMENTS));
   888	
   889			blk_queue_flag_set(QUEUE_FLAG_DISCARD, q);
   890		}
   891	
   892		if (virtio_has_feature(vdev, VIRTIO_BLK_F_WRITE_ZEROES)) {
   893			virtio_cread(vdev, struct virtio_blk_config,
   894				     max_write_zeroes_sectors, &v);
   895			blk_queue_max_write_zeroes_sectors(q, v ? v : UINT_MAX);
   896		}
   897	
   898		virtblk_update_capacity(vblk, false);
   899		virtio_device_ready(vdev);
   900	
   901		device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
   902		return 0;
   903	
   904	out_free_tags:
   905		blk_mq_free_tag_set(&vblk->tag_set);
   906	out_free_vq:
   907		vdev->config->del_vqs(vdev);
   908		kfree(vblk->vqs);
   909	out_free_vblk:
   910		kfree(vblk);
   911	out_free_index:
   912		ida_simple_remove(&vd_index_ida, index);
   913	out:
   914		return err;
   915	}
   916	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64549 bytes --]

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Jason Wang]

在 2021/7/15 下午5:47, Yury Kamenev 写道:
> Signed-off-by: Yury Kamenev <damtev@yandex-team.ru>


I think we need a better commit log here.

And why do we need a Kconfig for this? If there's a good reason, I guess 
the right approach is to invent something in the virtio core (via /sys)?

Thanks


> ---
>   .../admin-guide/kernel-parameters.txt         |  3 +++
>   drivers/block/Kconfig                         |  7 +++++
>   drivers/block/virtio_blk.c                    | 26 +++++++++++++++++++
>   include/uapi/linux/virtio_blk.h               |  2 ++
>   4 files changed, 38 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index bdb22006f713..941bdaf5c167 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6076,6 +6076,9 @@
>   			brightness level.
>   			default: 1
>
> +	virtiopartscan
> +		Enable virtio block device partition scanning omission based on VIRTIO_BLK_F_NO_PART_SCAN feature flag.
> +
>   	virtio_mmio.device=
>   			[VMMIO] Memory mapped virtio (platform) device.
>
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 63056cfd4b62..69ecd3fd7037 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -399,6 +399,13 @@ config VIRTIO_BLK
>   	  This is the virtual block driver for virtio.  It can be used with
>             QEMU based VMMs (like KVM or Xen).  Say Y or M.
>
> +config VIRTIO_BLK_NO_PART_SCAN
> +	bool "Disable partition scanning for devices with no partitions"
> +	depends on VIRTIO_BLK
> +	help
> +	  Disable partition scanning for devices with no partitions.
> +	  Can reduce the kernel start time for tiny systems like squashfs images.
> +
>   config BLK_DEV_RBD
>   	tristate "Rados block device (RBD)"
>   	depends on INET && BLOCK
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4b49df2dfd23..479711d3791c 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -692,6 +692,19 @@ static const struct blk_mq_ops virtio_mq_ops = {
>   static unsigned int virtblk_queue_depth;
>   module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>
> +#ifndef MODULE
> +#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
> +static int partitions_scanning_disable __read_mostly;
> +
> +static int __init partitions_scanning_setup(char *__unused)
> +{
> +	partitions_scanning_disable = 1;
> +	return 1;
> +}
> +__setup("nopartscan", partitions_scanning_setup);
> +#endif
> +#endif
> +
>   static int virtblk_probe(struct virtio_device *vdev)
>   {
>   	struct virtio_blk *vblk;
> @@ -790,6 +803,13 @@ static int virtblk_probe(struct virtio_device *vdev)
>   	vblk->disk->flags |= GENHD_FL_EXT_DEVT;
>   	vblk->index = index;
>
> +#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
> +	if (unlikely(partitions_scanning_disable))
> +		/* disable partitions scanning if it was stated in virtio features*/
> +		if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PART_SCAN))
> +			vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
> +#endif
> +
>   	/* configure queue flush support */
>   	virtblk_update_cache_mode(vdev);
>
> @@ -966,6 +986,9 @@ static unsigned int features_legacy[] = {
>   	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>   	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>   	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
> +	VIRTIO_BLK_F_NO_PART_SCAN,
> +#endif
>   }
>   ;
>   static unsigned int features[] = {
> @@ -973,6 +996,9 @@ static unsigned int features[] = {
>   	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>   	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>   	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +#ifdef CONFIG_VIRTIO_BLK_NO_PART_SCAN
> +	VIRTIO_BLK_F_NO_PART_SCAN,
> +#endif
>   };
>
>   static struct virtio_driver virtio_blk = {
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index d888f013d9ff..9b381675342a 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>   #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>   #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>   #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_NO_PART_SCAN	16	/* Disable partition scanning */
>
>   /* Legacy feature bits */
>   #ifndef VIRTIO_BLK_NO_LEGACY
> --
> 2.24.3 (Apple Git-128)
>

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Iurii Kamenev]

Thanks for your remark. I guess it is possible, I will try to rewrite it 
that way.

24.05.2021 22:41, Paolo Bonzini пишет:
> On 24/05/21 21:34, Юрий Каменев wrote:
>> Hi
>>
>>     Is your goal to avoid accidentally detecting partitions because it's
>>     confusing when that happens?
>>
>> The main goal is reducing the kernel start time. It might be use 
>> useful in tiny systems that use, for example, squashfs images with 
>> certainly no partitions. Disabling partitions scanning for these 
>> images can save a few tens of milliseconds which can be a significant 
>> acceleration for starting such systems.
>
> Perhaps that could be configured in the image, for example in the 
> kernel command line?
>
> Paolo
>
>> 24.05.2021, 17:29, "Stefan Hajnoczi" <stefanha@redhat.com>:
>>
>>     On Thu, May 20, 2021 at 04:39:08PM +0300, Yury Kamenev wrote:
>>
>>     Hi,
>>     Is there a VIRTIO spec change for the new VIRTIO_BLK_F_NO_PS feature
>>     bit? Please send one:
>> https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback
>> <https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback>
>>
>>     GENHD_FL_NO_PART_SCAN is not used much in other drivers. This 
>> makes me
>>     wonder if the same use case is addressed through other means with 
>> SCSI,
>>     NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether
>>     adding a bit to disable partition scanning for a virtio-blk fits 
>> into
>>     the big picture?
>>
>>     Is your goal to avoid accidentally detecting partitions because it's
>>     confusing when that happens?
>>
>>     VIRTIO is currently undergoing auditing and changes to support 
>> untrusted
>>     devices. From that perspective adding a device feature bit to 
>> disable
>>     partition scanning does not help protect the guest from an untrusted
>>     disk. The guest cannot trust the device, instead the guest itself 
>> would
>>     need to be configured to avoid partition scanning of untrusted 
>> devices.
>>
>>     Stefan
>>
>>           Signed-off-by: Yury Kamenev <damtev@yandex-team.ru
>>         <mailto:damtev@yandex-team.ru>>
>>           ---
>>            drivers/block/virtio_blk.c | 6 ++++++
>>            include/uapi/linux/virtio_blk.h | 1 +
>>            2 files changed, 7 insertions(+)
>>
>>           diff --git a/drivers/block/virtio_blk.c
>>         b/drivers/block/virtio_blk.c
>>           index b9fa3ef5b57c..17edcfee2208 100644
>>           --- a/drivers/block/virtio_blk.c
>>           +++ b/drivers/block/virtio_blk.c
>>           @@ -799,6 +799,10 @@ static int virtblk_probe(struct
>>         virtio_device *vdev)
>>                    vblk->disk->flags |= GENHD_FL_EXT_DEVT;
>>                    vblk->index = index;
>>
>>           + /*Disable partitions scanning for no-partitions block*/
>>
>>
>>     Formatting cleanup and rephrasing:
>>
>>        /* Disable partition scanning for devices with no partitions */
>>
>>           + if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PS))
>>
>>
>>     I suggest user a more obvious name:
>>
>>        VIRTIO_BLK_F_NO_PART_SCAN
>>
>>           + vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
>>           +
>>                    /* configure queue flush support */
>>                    virtblk_update_cache_mode(vdev);
>>
>>           @@ -977,6 +981,7 @@ static unsigned int features_legacy[] = {
>>                    VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>>                    VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY,
>>         VIRTIO_BLK_F_CONFIG_WCE,
>>                    VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
>>         VIRTIO_BLK_F_WRITE_ZEROES,
>>           + VIRTIO_BLK_F_NO_PS,
>>            }
>>            ;
>>            static unsigned int features[] = {
>>           @@ -984,6 +989,7 @@ static unsigned int features[] = {
>>                    VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>>                    VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY,
>>         VIRTIO_BLK_F_CONFIG_WCE,
>>                    VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
>>         VIRTIO_BLK_F_WRITE_ZEROES,
>>           + VIRTIO_BLK_F_NO_PS,
>>            };
>>
>>            static struct virtio_driver virtio_blk = {
>>           diff --git a/include/uapi/linux/virtio_blk.h
>>         b/include/uapi/linux/virtio_blk.h
>>           index d888f013d9ff..f197d07afb05 100644
>>           --- a/include/uapi/linux/virtio_blk.h
>>           +++ b/include/uapi/linux/virtio_blk.h
>>           @@ -40,6 +40,7 @@
>>            #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
>>            #define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
>>            #define VIRTIO_BLK_F_WRITE_ZEROES 14 /* WRITE ZEROES is
>>         supported */
>>           +#define VIRTIO_BLK_F_NO_PS 16 /* No partitions */
>>
>>            /* Legacy feature bits */
>>            #ifndef VIRTIO_BLK_NO_LEGACY
>>           --
>>           2.24.3 (Apple Git-128)
>>
>

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Paolo Bonzini]

On 24/05/21 21:34, Юрий Каменев wrote:
> Hi
> 
>     Is your goal to avoid accidentally detecting partitions because it's
>     confusing when that happens?
> 
> The main goal is reducing the kernel start time. It might be use useful 
> in tiny systems that use, for example, squashfs images with certainly no 
> partitions. Disabling partitions scanning for these images can save a 
> few tens of milliseconds which can be a significant acceleration for 
> starting such systems.

Perhaps that could be configured in the image, for example in the kernel 
command line?

Paolo

> 24.05.2021, 17:29, "Stefan Hajnoczi" <stefanha@redhat.com>:
> 
>     On Thu, May 20, 2021 at 04:39:08PM +0300, Yury Kamenev wrote:
> 
>     Hi,
>     Is there a VIRTIO spec change for the new VIRTIO_BLK_F_NO_PS feature
>     bit? Please send one:
>     https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback
>     <https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback>
> 
>     GENHD_FL_NO_PART_SCAN is not used much in other drivers. This makes me
>     wonder if the same use case is addressed through other means with SCSI,
>     NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether
>     adding a bit to disable partition scanning for a virtio-blk fits into
>     the big picture?
> 
>     Is your goal to avoid accidentally detecting partitions because it's
>     confusing when that happens?
> 
>     VIRTIO is currently undergoing auditing and changes to support untrusted
>     devices. From that perspective adding a device feature bit to disable
>     partition scanning does not help protect the guest from an untrusted
>     disk. The guest cannot trust the device, instead the guest itself would
>     need to be configured to avoid partition scanning of untrusted devices.
> 
>     Stefan
> 
>           Signed-off-by: Yury Kamenev <damtev@yandex-team.ru
>         <mailto:damtev@yandex-team.ru>>
>           ---
>            drivers/block/virtio_blk.c | 6 ++++++
>            include/uapi/linux/virtio_blk.h | 1 +
>            2 files changed, 7 insertions(+)
> 
>           diff --git a/drivers/block/virtio_blk.c
>         b/drivers/block/virtio_blk.c
>           index b9fa3ef5b57c..17edcfee2208 100644
>           --- a/drivers/block/virtio_blk.c
>           +++ b/drivers/block/virtio_blk.c
>           @@ -799,6 +799,10 @@ static int virtblk_probe(struct
>         virtio_device *vdev)
>                    vblk->disk->flags |= GENHD_FL_EXT_DEVT;
>                    vblk->index = index;
> 
>           + /*Disable partitions scanning for no-partitions block*/
> 
> 
>     Formatting cleanup and rephrasing:
> 
>        /* Disable partition scanning for devices with no partitions */
> 
>           + if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PS))
> 
> 
>     I suggest user a more obvious name:
> 
>        VIRTIO_BLK_F_NO_PART_SCAN
> 
>           + vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
>           +
>                    /* configure queue flush support */
>                    virtblk_update_cache_mode(vdev);
> 
>           @@ -977,6 +981,7 @@ static unsigned int features_legacy[] = {
>                    VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>                    VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY,
>         VIRTIO_BLK_F_CONFIG_WCE,
>                    VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
>         VIRTIO_BLK_F_WRITE_ZEROES,
>           + VIRTIO_BLK_F_NO_PS,
>            }
>            ;
>            static unsigned int features[] = {
>           @@ -984,6 +989,7 @@ static unsigned int features[] = {
>                    VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>                    VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY,
>         VIRTIO_BLK_F_CONFIG_WCE,
>                    VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
>         VIRTIO_BLK_F_WRITE_ZEROES,
>           + VIRTIO_BLK_F_NO_PS,
>            };
> 
>            static struct virtio_driver virtio_blk = {
>           diff --git a/include/uapi/linux/virtio_blk.h
>         b/include/uapi/linux/virtio_blk.h
>           index d888f013d9ff..f197d07afb05 100644
>           --- a/include/uapi/linux/virtio_blk.h
>           +++ b/include/uapi/linux/virtio_blk.h
>           @@ -40,6 +40,7 @@
>            #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
>            #define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
>            #define VIRTIO_BLK_F_WRITE_ZEROES 14 /* WRITE ZEROES is
>         supported */
>           +#define VIRTIO_BLK_F_NO_PS 16 /* No partitions */
> 
>            /* Legacy feature bits */
>            #ifndef VIRTIO_BLK_NO_LEGACY
>           --
>           2.24.3 (Apple Git-128)
> 

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Ulf Hansson]

On Mon, 24 May 2021 at 16:57, Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, May 24, 2021 at 03:29:22PM +0100, Stefan Hajnoczi wrote:
> > GENHD_FL_NO_PART_SCAN is not used much in other drivers. This makes me
> > wonder if the same use case is addressed through other means with SCSI,
> > NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether
> > adding a bit to disable partition scanning for a virtio-blk fits into
> > the big picture?
> >
> > Is your goal to avoid accidentally detecting partitions because it's
> > confusing when that happens?
>
> I'm really confused what the use case is here.  GENHD_FL_NO_PART_SCAN
> has four users:
>
>  - the block core setting it for hidden devices, for which the concept
>    of paritions doesn't make sense.  Looking back this should have never
>    used GENHD_FL_NO_PART_SCAN, and instead the partition scanning code
>    should just check GENHD_FL_HIDDEN as well.
>  - mmc uses it for boot partitions and rpmb.  I'm not even sure how
>    these can be exposed as block devices as they don't require block
>    granularity access IIRC, but if the allow block layer access there
>    is no reason to ever set these flags.

For RPMB, we have converted them into char devices, thus
GENHD_FL_NO_PART_SCAN is never set for them. The code needs a cleanup
to clarify this.

When it comes to eMMC boot partitions, those can be read/written to as
any other block device. Although, it's unlikely that they need
partitions as they are usually very small, 512Kb or 2MB in that
ballpark. At least, that was the thinking behind it when we added
GENHD_FL_NO_PART_SCAN for them.

If you want to drop GENHD_FL_NO_PART_SCAN for eMMC boot partitions, I
don't think it will be an issue.

>  - loop is a bit of a mess.  IIRC the story is that originally the
>    loop device did not support partitions, then in 2008 support for
>    partitions was added by partitioning the minor number space, and
>    then in 2011 support for partitions without that parameter was
>    added using a new flag in the loop device creation ioctl that uses
>    the extended dev_t space added since.  But even that might be
>    something we can handled without that flag without breaking the
>    userspace ABI
>  - m64card sets it for no good reason at all
>
> In other words: in a perfect would GENHD_FL_NO_PART_SCAN would not
> exist, and it certainly should not be added to a new driver, never
> mind a protocol.
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Kind regards
Uffe

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Christoph Hellwig]

On Mon, May 24, 2021 at 03:29:22PM +0100, Stefan Hajnoczi wrote:
> GENHD_FL_NO_PART_SCAN is not used much in other drivers. This makes me
> wonder if the same use case is addressed through other means with SCSI,
> NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether
> adding a bit to disable partition scanning for a virtio-blk fits into
> the big picture?
> 
> Is your goal to avoid accidentally detecting partitions because it's
> confusing when that happens?

I'm really confused what the use case is here.  GENHD_FL_NO_PART_SCAN
has four users:

 - the block core setting it for hidden devices, for which the concept
   of paritions doesn't make sense.  Looking back this should have never
   used GENHD_FL_NO_PART_SCAN, and instead the partition scanning code
   should just check GENHD_FL_HIDDEN as well.
 - mmc uses it for boot partitions and rpmb.  I'm not even sure how
   these can be exposed as block devices as they don't require block
   granularity access IIRC, but if the allow block layer access there
   is no reason to ever set these flags.
 - loop is a bit of a mess.  IIRC the story is that originally the
   loop device did not support partitions, then in 2008 support for
   partitions was added by partitioning the minor number space, and
   then in 2011 support for partitions without that parameter was
   added using a new flag in the loop device creation ioctl that uses
   the extended dev_t space added since.  But even that might be
   something we can handled without that flag without breaking the
   userspace ABI
 - m64card sets it for no good reason at all

In other words: in a perfect would GENHD_FL_NO_PART_SCAN would not
exist, and it certainly should not be added to a new driver, never
mind a protocol.

Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3246 bytes --]

On Thu, May 20, 2021 at 04:39:08PM +0300, Yury Kamenev wrote:

Hi,
Is there a VIRTIO spec change for the new VIRTIO_BLK_F_NO_PS feature
bit? Please send one:
https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback

GENHD_FL_NO_PART_SCAN is not used much in other drivers. This makes me
wonder if the same use case is addressed through other means with SCSI,
NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether
adding a bit to disable partition scanning for a virtio-blk fits into
the big picture?

Is your goal to avoid accidentally detecting partitions because it's
confusing when that happens?

VIRTIO is currently undergoing auditing and changes to support untrusted
devices. From that perspective adding a device feature bit to disable
partition scanning does not help protect the guest from an untrusted
disk. The guest cannot trust the device, instead the guest itself would
need to be configured to avoid partition scanning of untrusted devices.

Stefan

> Signed-off-by: Yury Kamenev <damtev@yandex-team.ru>
> ---
>  drivers/block/virtio_blk.c      | 6 ++++++
>  include/uapi/linux/virtio_blk.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index b9fa3ef5b57c..17edcfee2208 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -799,6 +799,10 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	vblk->disk->flags |= GENHD_FL_EXT_DEVT;
>  	vblk->index = index;
>  
> +	/*Disable partitions scanning for no-partitions block*/

Formatting cleanup and rephrasing:

  /* Disable partition scanning for devices with no partitions */

> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PS))

I suggest user a more obvious name:

  VIRTIO_BLK_F_NO_PART_SCAN

> +		vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
> +
>  	/* configure queue flush support */
>  	virtblk_update_cache_mode(vdev);
>  
> @@ -977,6 +981,7 @@ static unsigned int features_legacy[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +	VIRTIO_BLK_F_NO_PS,
>  }
>  ;
>  static unsigned int features[] = {
> @@ -984,6 +989,7 @@ static unsigned int features[] = {
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
>  	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>  	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
> +	VIRTIO_BLK_F_NO_PS,
>  };
>  
>  static struct virtio_driver virtio_blk = {
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index d888f013d9ff..f197d07afb05 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
>  #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
>  #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
>  #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_NO_PS      16      /* No partitions */
>  
>  /* Legacy feature bits */
>  #ifndef VIRTIO_BLK_NO_LEGACY
> -- 
> 2.24.3 (Apple Git-128)
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/1] virtio: disable partitions scanning for no partitions block

[Yury Kamenev]

Signed-off-by: Yury Kamenev <damtev@yandex-team.ru>
---
 drivers/block/virtio_blk.c      | 6 ++++++
 include/uapi/linux/virtio_blk.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b9fa3ef5b57c..17edcfee2208 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -799,6 +799,10 @@ static int virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->flags |= GENHD_FL_EXT_DEVT;
 	vblk->index = index;
 
+	/*Disable partitions scanning for no-partitions block*/
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_NO_PS))
+		vblk->disk->flags |= GENHD_FL_NO_PART_SCAN;
+
 	/* configure queue flush support */
 	virtblk_update_cache_mode(vdev);
 
@@ -977,6 +981,7 @@ static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_NO_PS,
 }
 ;
 static unsigned int features[] = {
@@ -984,6 +989,7 @@ static unsigned int features[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_NO_PS,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9ff..f197d07afb05 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_NO_PS      16      /* No partitions */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
-- 
2.24.3 (Apple Git-128)