[RFC] set_blocksize() in pktcdvd (was Re: [PATCH vfs.all 22/26] block: stash a bdev_file to read/write raw blcok_device)

Commit Message

Al Viro April 17, 2024, 4:35 a.m. UTC

On Tue, Apr 16, 2024 at 07:32:53AM +0100, Al Viro wrote:

> drivers/block/pktcdvd.c:2285:           set_blocksize(disk->part0, CD_FRAMESIZE);

	We had hardsect_size set to that 2Kb from the very beginning
(well, logical_block_size these days).	And the first ->open() is
(and had been since before the pktcdvd went into mainline) followed by
setting block size anyway, so any effects of that set_blocksize() had
always been lost.  Candidate block sizes start at logical_block_size...
Rudiment of something from 2000--2004 when it existed out of tree?
<checks>  That logic into the tree in 2.5.13; May 2002...

	AFAICS, this one can be simply removed.  Jens, do you have
any objections to that?  It's safe, but really pointless...

> drivers/block/pktcdvd.c:2529:   set_blocksize(file_bdev(bdev_file), CD_FRAMESIZE);

	This, OTOH, is not safe at all - we don't have the underlying device
exclusive, and it's possible that it is in use with e.g. 4Kb block size (e.g.
from ext* read-only mount, with 4Kb blocks).  This set_blocksize() will screw
the filesystem very badly - block numbers mapping to LBA will change, for starters.

	We are setting a pktcdvd device up here, and that set_blocksize()
is done to the underlying device.  It does *not* prevent changes of block
size of the underlying device by the time we actually open the device
we'd set up - set_blocksize() in ->open() is done to pktcdvd device,
not the underlying one.  So... what is it for?

	It might make sense to move it into ->open(), where we do have
the underlying device claimed.	But doing that at the setup time looks
very odd...

	Do you have any objections against this:

commit d1d93f2c26f70fbcd714615d1a3ea7a104fc0f43
Author: Al Viro <[e-mail]>
Date:   Wed Apr 17 00:28:03 2024 -0400

    pktcdvd: sort set_blocksize() calls out
    
    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 <[e-mail]>

Patch

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);