On 21-01-04 00:57:07, Chaitanya Kulkarni wrote:
> On 12/30/20 08:03, Minwoo Im wrote:
> > Let's say, for example of NVMe device, Format command to change out
> > LBA format to another logical block size and BLKRRPART to re-read
> > partition information with a same file descriptor like:
> >
> > fd = open("/dev/nvme0n1", O_RDONLY);
> >
> > nvme_format(fd, ...);
> > if (ioctl(fd, BLKRRPART) < 0)
> > ..
> >
> > In this case, ioctl causes invalid Read operations which are triggered
> > by buffer_head I/O path to re-read partition information. This is
> > because it's still playing around with i_blksize and i_blkbits. So,
> > 512 -> 4096 -> 512 logical block size changes will cause an under-flowed
> > length of Read operations.
> >
> > Case for NVMe:
> > (LBAF 1 512B, LBAF 0 4K logical block size)
> >
> > nvme format /dev/nvme0n1 --lbaf=1 --force # to 512B LBA
> > nvme format /dev/nvme0n1 --lbaf=0 --force # to 4096B LBA
> >
> > [dmesg-snip]
> > [ 10.771740] blk_update_request: operation not supported error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
> > [ 10.780262] Buffer I/O error on dev nvme0n1, logical block 0, async page read
> >
> > [event-snip]
> > kworker/0:1H-56 [000] .... 913.456922: nvme_setup_cmd: nvme0: disk=nvme0n1, qid=1, cmdid=216, nsid=1, flags=0x0, meta=0x0, cmd=(nvme_cmd_read slba=0, len=65535, ctrl=0x0, dsmgmt=0, reftag=0)
> > ksoftirqd/0-9 [000] .Ns. 916.566351: nvme_complete_rq: nvme0: disk=nvme0n1, qid=1, cmdid=216, res=0x0, retries=0, flags=0x0, status=0x4002
> >
> > As the previous discussion [1], this patch introduced a gendisk flag
> > to indicate that block size has been changed in the runtime. This flag
> > is set when logical block size is changed in the runtime in the block
> > layer. It will be cleared when the file descriptor for the
> > block devie is opened again through __blkdev_get() which updates the block
> > size via set_init_blocksize().
> >
> > This patch rejects I/O from the path of add_partitions() to avoid
> > issuing invalid Read operations to device. It also sets a flag to
> > gendisk in blk_queue_logical_block_size to minimize caller-side updates.
> >
> > [1] https://lore.kernel.org/linux-nvme/20201223183143.GB13354@localhost.localdomain/T/#t
> >
> > Signed-off-by: Minwoo Im <minwoo.im.dev@xxxxxxxxx>
> Rewrite the change-log similar to what we have in the repo and fix
> the spelling mistakes. Add a cover-letter to explain the testcase
> and the execution effect, also I'd move discussion link into
> cover-letter.
Thanks for your time. Will prepare V3 with proper change logs and cover
letter to explain issue and testcase.
Thanks,
