Re: [PATCH vfs.all 22/26] block: stash a bdev_file to read/write raw blcok_device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 12, 2024 at 09:38:16AM +0800, Yu Kuai wrote:

> There really is a long history here. The beginning of the attempt to try
> removing the filed 'bd_inode' is that I want to make a room from the
> first cacheline(64 bytes) for a new 'unsigned long flags' field because
> we keep adding new 'bool xxx' field [1]. And adding a new 'bd_mapping'
> field will make that impossible.

Why does it need to be unsigned long?  dev_t is 32bit; what you need
is to keep this
        bool                    bd_read_only;   /* read-only policy */
	u8                      bd_partno;
	bool                    bd_write_holder;
	bool                    bd_has_submit_bio;

from blowing past u32.  Sure, you can't use test_bit() et.al. with u16,
but what's wrong with explicit bitwise operations?  You need some protection
for multiple writers, but you need it anyway - e.g. this
        if (bdev->bd_disk->fops->set_read_only) {
		ret = bdev->bd_disk->fops->set_read_only(bdev, n);
		if (ret)
			return ret;
	}
	bdev->bd_read_only = n;
will need the exclusion over the entire "call ->set_read_only() and set
the flag", not just for setting the flag itself.

And yes, it's a real-world bug - two threads calling BLKROSET on the
same opened file can race, with inconsistency between the flag and
whatever state ->set_read_only() modifies.

AFAICS, ->bd_write_holder is (apparently) relying upon ->open_mutex.
Whether it would be a good solution for ->bd_read_only is a question
to block folks, but some exclusion is obviously needed.

Let's sort that out, rather than papering it over with set_bit() et.al.




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux