Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: aZuT+LSkzSybbc9xlQBBj5l9e/B1NFgG9506tKcN3kgKqTzeUqOCgR7ndspdiP1I1Xx2LDkGl2
 U2ervnrKAjjdvTb/YH0yeiVEdgu94jx/olvDz0uqmX7QSXACe0P9T/znMTROffvqUWxpt1Il5r
 BT66JMcsMBO6WS0qGrQPHs1P/6nOU0F5cCFgOrGzi63qXKYdhctgDTQQOaAI5E2quNFfOfd66j
 3FT3PL3l3yKZKyOROyP0FvMGSAewaZrUGcZsitGmhefL5jTB32rpqKZ94xjSJjpPJwVC2RBOuq
 KrU=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Minwoo Im <minwoo.im.dev@gmail.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
CC:     Jens Axboe <axboe@kernel.dk>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [RFC V2] block: reject I/O for same fd if block size changed
Thread-Topic: [RFC V2] block: reject I/O for same fd if block size changed
Thread-Index: AQHW3sVYE0D3BnsJEkakKhpEGYvfNw==
Date:   Mon, 4 Jan 2021 00:57:07 +0000
Message-ID: <BYAPR04MB49659FFF1A820C9E7BBC1D0386D20@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20201230160129.23731-1-minwoo.im.dev@gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 95144d6f-e103-41df-53ef-08d8b04ba90b
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Jan 2021 00:57:07.7323
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 1R4mMaG6QV0AX3rNOlphn9AyOxb5xi48x58571LXrIOH3NkxAS2knAKQ+cuuP0EeF5m+Yuql4KTd9AUKxcZPXha+oBhgKOmC5aqfLRbLrs0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR04MB7438
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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