Re: [PATCH 2/2] nbd: add netlink reconfigure resize support v3

[Mike Christie <mchristi@redhat.com>]

On 06/13/2019 12:01 PM, Josef Bacik wrote:
> On Wed, May 29, 2019 at 03:16:06PM -0500, Mike Christie wrote:
>> If the device is setup with ioctl we can resize the device after the
>> initial setup, but if the device is setup with netlink we cannot use the
>> resize related ioctls and there is no netlink reconfigure size ATTR
>> handling code.
>>
>> This patch adds netlink reconfigure resize support to match the ioctl
>> interface.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
> 
> Sorry I missed this too, but I think there's a problem with this actually.
> 
>> ---
>>
>> V3;
>> - If the device size or block size has not changed do not call
>> nbd_size_set.
>>
>> V2:
>> - Merge reconfig and connect resize related code to helper and avoid
>> multiple nbd_size_set calls.
>>
>>  drivers/block/nbd.c | 48 ++++++++++++++++++++++++++++++---------------
>>  1 file changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 236253fbf455..9486555e6391 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -1685,6 +1685,30 @@ nbd_device_policy[NBD_DEVICE_ATTR_MAX + 1] = {
>>  	[NBD_DEVICE_CONNECTED]		=	{ .type = NLA_U8 },
>>  };
>>  
>> +static int nbd_genl_size_set(struct genl_info *info, struct nbd_device *nbd)
>> +{
>> +	struct nbd_config *config = nbd->config;
>> +	u64 bsize = config->blksize;
>> +	u64 bytes = config->bytesize;
>> +
>> +	if (info->attrs[NBD_ATTR_SIZE_BYTES])
>> +		bytes = nla_get_u64(info->attrs[NBD_ATTR_SIZE_BYTES]);
>> +
>> +	if (info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]) {
>> +		bsize = nla_get_u64(info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]);
>> +		if (!bsize)
>> +			bsize = NBD_DEF_BLKSIZE;
>> +		if (!nbd_is_valid_blksize(bsize)) {
>> +			printk(KERN_ERR "Invalid block size %llu\n", bsize);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	if (bytes != config->bytesize || bsize != config->blksize)
>> +		nbd_size_set(nbd, bsize, div64_u64(bytes, bsize));
> 
> This part won't actually update the bdev if there already is one because
> nbd->task_recv is NULL for netlink related devices.  Probably need to fix that

I'm not sure I understand this part of the comment. For netlink we do:

nbd_genl_connect -> nbd_start_device:

nbd_start_device()
{
    .....
    nbd->task_recv = current;


> to update the bdev unconditionally, and then just see if bdget_disk() returns
> NULL in nbd_size_update.  Also I hate myself for how many size update functions
> there are.  Thanks,
> 
> Josef
>