Re: [PATCH] loop: properly observe rotational flag of underlying device

["Holger Hoffstätte" <holger.hoffstaette@googlemail.com>]

On 11/11/15 22:29, Jens Axboe wrote:
> On 11/11/2015 08:21 AM, Holger Hoffstätte wrote:
>>
>> The loop driver always declares the rotational flag of its device as
>> rotational, even when the device of the mapped file is nonrotational,
>> as is the case with SSDs or on tmpfs. This can confuse filesystem tools
>> which are SSD-aware; in my case I frequently forget to tell mkfs.btrfs
>> that my loop device on tmpfs is nonrotational, and that I really don't
>> need any automatic metadata redundancy.
>>
>> The attached patch fixes this by introspecting the rotational flag of the
>> mapped file's underlying block device, if it exists. If the mapped file's
>> filesystem has no associated block device - as is the case on e.g. tmpfs -
>> we assume nonrotational storage. If there is a better way to identify such
>> non-devices I'd love to hear them.
>>
>> Signed-off-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>
>> ---
>>   drivers/block/loop.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 423f4ca..2984aca 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -843,6 +843,24 @@ static void loop_config_discard(struct loop_device *lo)
>>       queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
>>   }
>>
>> +static void loop_update_rotational(struct loop_device *lo)
>> +{
>> +    struct file *file = lo->lo_backing_file;
>> +    struct inode *file_inode = file->f_mapping->host;
>> +    struct block_device *file_bdev = file_inode->i_sb->s_bdev;
>> +    struct request_queue *q = lo->lo_queue;
>> +    bool nonrot = true;
>> +
>> +    /* not all filesystems (e.g. tmpfs) have a sb->s_bdev */
>> +    if (file_bdev)
>> +        nonrot = blk_queue_nonrot(bdev_get_queue(file_bdev));
>> +
>> +    if (nonrot)
>> +        queue_flag_set_unlocked(QUEUE_FLAG_NONROT, q);
>> +    else
>> +        queue_flag_clear_unlocked(QUEUE_FLAG_NONROT, q);
>> +}
> 
> Are we sure we want to change the default from rot to nonrot?

Well, that's why I asked for a better way to identify tmpfs. It took
me several hours to figure out that tmpfs doesn't have an s_bdev, and
could not find a better way than to assume that a superblock without
backing device is probably something virtual/nonrotational/nvm etc.

Alternatively I could look at sb->s_type and set nonrot for known fs
types, but that seemed too ugly - not to mention conceptually weird.

> Apart from that, looks good.

phew :)

thanks,
Holger