| Message ID | 20240427211032.GB1495312@ZenIV (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] bcache_register(): don't bother with set_blocksize() | expand |
On Sat, Apr 27, 2024 at 10:10:32PM +0100, Al Viro wrote: > 1) it doesn't make any sense to have ->open() call set_blocksize() on the > device being opened - the caller will override that anyway. > > 2) setting block size on underlying device, OTOH, ought to be done when > we are opening it exclusive - i.e. as part of pkt_open_dev(). Having > it done at setup time doesn't guarantee us anything about the state > at the time we start talking to it. Worse, if you happen to have > the underlying device containing e.g. ext2 with 4Kb blocks that > is currently mounted r/o, that set_blocksize() will confuse the hell > out of filesystem. I brought some of this before and didn't dare to touch it because I have no way of testing this code. The changes looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de> But I really wish we could find a dedicated tester for pktcdvd or just drop this code..
On Sat, Apr 27, 2024 at 10:10:32PM +0100, Al Viro wrote: > 1) it doesn't make any sense to have ->open() call set_blocksize() on the > device being opened - the caller will override that anyway. > > 2) setting block size on underlying device, OTOH, ought to be done when > we are opening it exclusive - i.e. as part of pkt_open_dev(). Having > it done at setup time doesn't guarantee us anything about the state > at the time we start talking to it. Worse, if you happen to have > the underlying device containing e.g. ext2 with 4Kb blocks that > is currently mounted r/o, that set_blocksize() will confuse the hell > out of filesystem. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Looks good to me, Reviewed-by: Christian Brauner <brauner@kernel.org>
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c index 21728e9ea5c3..05933f25b397 100644 --- a/drivers/block/pktcdvd.c +++ b/drivers/block/pktcdvd.c @@ -2215,6 +2215,7 @@ static int pkt_open_dev(struct pktcdvd_device *pd, bool write) } dev_info(ddev, "%lukB available on disc\n", lba << 1); } + set_blocksize(file_bdev(bdev_file), CD_FRAMESIZE); return 0; @@ -2278,11 +2279,6 @@ static int pkt_open(struct gendisk *disk, blk_mode_t mode) ret = pkt_open_dev(pd, mode & BLK_OPEN_WRITE); if (ret) goto out_dec; - /* - * needed here as well, since ext2 (among others) may change - * the blocksize at mount time - */ - set_blocksize(disk->part0, CD_FRAMESIZE); } mutex_unlock(&ctl_mutex); mutex_unlock(&pktcdvd_mutex); @@ -2526,7 +2522,6 @@ static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev) __module_get(THIS_MODULE); pd->bdev_file = bdev_file; - set_blocksize(file_bdev(bdev_file), CD_FRAMESIZE); atomic_set(&pd->cdrw.pending_bios, 0); pd->cdrw.thread = kthread_run(kcdrwd, pd, "%s", pd->disk->disk_name);
1) it doesn't make any sense to have ->open() call set_blocksize() on the device being opened - the caller will override that anyway. 2) setting block size on underlying device, OTOH, ought to be done when we are opening it exclusive - i.e. as part of pkt_open_dev(). Having it done at setup time doesn't guarantee us anything about the state at the time we start talking to it. Worse, if you happen to have the underlying device containing e.g. ext2 with 4Kb blocks that is currently mounted r/o, that set_blocksize() will confuse the hell out of filesystem. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/block/pktcdvd.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)