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.
