On Fri, Oct 25, 2024 at 02:36:41PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> When multiple partitions are used, you may want to enforce different
> subsets of the available write hints for each partition. Provide a
> bitmap attribute of the available write hints, and allow an admin to
> write a different mask to set the partition's allowed write hints.
Trying my best Greg impersonator voice: This needs to be documented
in Documentation/ABI/stable/sysfs-block.
That would have also helped me understanding it. AFAIK the split here
is an opt-in, which means the use case I explained in the previous
case would still not work out of the box, right?
> + max_write_hints = bdev_max_write_hints(bdev);
> + if (max_write_hints) {
> + int size = BITS_TO_LONGS(max_write_hints) * sizeof(long);
> +
> + bdev->write_hint_mask = kmalloc(size, GFP_KERNEL);
> + if (!bdev->write_hint_mask) {
> + free_percpu(bdev->bd_stats);
> + iput(inode);
> + return NULL;
> + }
> + memset(bdev->write_hint_mask, 0xff, size);
> + }
This could simply use bitmap_alloc(). Similarly the other uses
would probably benefit from using the bitmap API.
> + struct block_device *bdev = dev_to_bdev(dev);
> + unsigned short max_write_hints = bdev_max_write_hints(bdev);
> +
> + if (max_write_hints)
> + return sprintf(buf, "%*pb\n", max_write_hints, bdev->write_hint_mask);
> + else
> + return sprintf(buf, "0");
No need for the else. And if you write this as:
if (!max_write_hints)
return sprintf(buf, "0");
return sprintf(buf, "%*pb\n", max_write_hints, bdev->write_hint_mask);
you'd also avoid the overly long line.
> +
> +static ssize_t part_write_hint_mask_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct block_device *bdev = dev_to_bdev(dev);
> + unsigned short max_write_hints = bdev_max_write_hints(bdev);
> + unsigned long *new_mask;
> + int size;
> +
> + if (!max_write_hints)
> + return count;
> +
> + size = BITS_TO_LONGS(max_write_hints) * sizeof(long);
> + new_mask = kzalloc(size, GFP_KERNEL);
> + if (!new_mask)
> + return -ENOMEM;
> +
> + bitmap_parse(buf, count, new_mask, max_write_hints);
> + bitmap_copy(bdev->write_hint_mask, new_mask, max_write_hints);
What protects access to bdev->write_hint_mask?
