Re: [PATCH v4 5/8] block: implement async discard as io_uring cmd

[Pavel Begunkov <asml.silence@gmail.com>]

On 9/10/24 09:01, Christoph Hellwig wrote:
>> +	sector_t sector = start >> SECTOR_SHIFT;
>> +	sector_t nr_sects = len >> SECTOR_SHIFT;
>> +	struct bio *prev = NULL, *bio;
>> +	int err;
>> +
>> +	if (!bdev_max_discard_sectors(bdev))
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!(file_to_blk_mode(cmd->file) & BLK_OPEN_WRITE))
>> +		return -EBADF;
>> +	if (bdev_read_only(bdev))
>> +		return -EPERM;
>> +	err = blk_validate_byte_range(bdev, start, len);
>> +	if (err)
>> +		return err;
> 
> Based on the above this function is misnamed, as it validates sector_t
> range and not a byte range.

Start and len here are in bytes. What do you mean?


>> +	if (nowait && nr_sects > bio_discard_limit(bdev, sector))
>> +		return -EAGAIN;
>> +
>> +	err = filemap_invalidate_pages(bdev->bd_mapping, start,
>> +					start + len - 1, nowait);
>> +	if (err)
>> +		return err;
>> +
>> +	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
>> +		if (nowait)
>> +			bio->bi_opf |= REQ_NOWAIT;
>> +		prev = bio_chain_and_submit(prev, bio);
>> +	}
>> +	if (!prev)
>> +		return -EAGAIN;
> 
> If a user changes the max_discard value between the check above and
> the loop here this is racy.

If the driver randomly changes it, it's racy either way. What do
you want to protect against?

>> +sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);
> 
> And to be honest, I'd really prefer to not have bio_discard_limit
> exposed.  Certainly not outside a header private to block/.

Which is the other reason why first versions were putting down
a bio seeing that there is more to be done for nowait, which
you didn't like. I can return back to it or narrow the scopre.

>> +
>>   #endif /* __LINUX_BIO_H */
>> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
>> index 753971770733..7ea41ca97158 100644
>> --- a/include/uapi/linux/fs.h
>> +++ b/include/uapi/linux/fs.h
>> @@ -208,6 +208,8 @@ struct fsxattr {
>>    * (see uapi/linux/blkzoned.h)
>>    */
>>   
>> +#define BLOCK_URING_CMD_DISCARD			_IO(0x12,137)
> 
> Whitespace after the comma please. 

That appears to be the "code style" of all BLK ioctls.

> Also why start at 137?  A comment
> would generally be pretty useful as well.

There is a comment, 2 lines above the new define.

/*
  * A jump here: 130-136 are reserved for zoned block devices
  * (see uapi/linux/blkzoned.h)
  */

Is that your concern?

> Also can we have a include/uapi/linux/blkdev.h for this instead of
> bloating fs.h that gets included just about everywhere?
I don't think it belongs to this series. Regardless, how do you
see it? The new file can have just those several new definitions,
in fs.h we'd have another comment why there is another empty range,
but I don't think it's worth it at all.

Another option is to move there everything block related, and make
fs.h include blkdev.h, which can always be done on top.

-- 
Pavel Begunkov