Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info

From: Hans de Goede <hdegoede@redhat.com>
To: Christoph Hellwig <hch@lst.de>, Jan Kara <jack@suse.cz>
Cc: axboe@kernel.dk, yuyufen@huawei.com, tj@kernel.org,
	bvanassche@acm.org, tytso@mit.edu, gregkh@linuxfoundation.org,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/8] bdi: add a ->dev_name field to struct backing_dev_info
Date: Mon, 20 Apr 2020 13:41:57 +0200	[thread overview]
Message-ID: <e02b7cdc-f29a-916c-d923-224a1b312485@redhat.com> (raw)
In-Reply-To: <20200417130135.GB5053@lst.de>

Hi,

<A lot of context has been trimmed here before I got added to the Cc, so
  I'm assuming that we are talking about the vboxsf code here.>

On 4/17/20 3:01 PM, Christoph Hellwig wrote:
> On Fri, Apr 17, 2020 at 10:59:09AM +0200, Jan Kara wrote:
>>> -	dev = device_create_vargs(bdi_class, NULL, MKDEV(0, 0), bdi, fmt, args);
>>> +	vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
>>> +	dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
>>>   	if (IS_ERR(dev))
>>>   		return PTR_ERR(dev);
>>>   
>>
>> This can have a sideeffect not only bdi->dev_name will be truncated to 64
>> chars (which generally doesn't matter) but possibly also kobject name will
>> be truncated in the same way.  Which may have user visible effects. E.g.
>> for fs/vboxsf 64 chars need not be enough. So shouldn't we rather do it the
>> other way around - i.e., let device_create_vargs() create the device name
>> and then copy to bdi->dev_name whatever fits?
> 
> I think having them mismatch is worse, as the kobject name is what
> people look for.  Hans, do you know what fc->source typicall contains
> and if there is much of a problem if it gets truncated/  Can we switch
> to something else that is guranteed to be 64 charaters or less for the
> bdi name?

It contains the name the user has given to the shared-folder when
exporting it from the host/hypervisor. Typically this will be the
last element of the directory path, e.g. if I export /home/hans/projects/linux
then the default/suggested share name and this the source name to pass to
the host when mounting the shared-folder will be "linux". But the user can
put anything there.

AFAICT for vboxsf the bdi-name can be anything as long as it is unique, hence
the "vboxsf-" prefix to make this unique vs other block-devices and the
".%d" postfix is necessary because the same export can be mounted multiple
times (without using bind mounts), see:
https://github.com/jwrdegoede/vboxsf/issues/3

The presence of the source inside the bdi-name is only for informational
purposes really, so truncating that should be fine, maybe switch to:

"vboxsf%d-%s" as format string and swap the sbi->bdi_id and fc->source
in the args, then if we truncate anything it will be the source (which
as said is only there for informational purposes) and the name will
still be guaranteed to be unique.

Regards,

Hans