Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Pranay Srivastava]

On Thu, May 12, 2016 at 2:49 PM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:
> Hi,
>
> On Wednesday 11 May 2016 11:18:46 Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>> wait for it's users to finish.
>>
>> This is more required when filesystem(s) like
>> ext2 or ext3 don't expect their buffer heads to
>> disappear while the filesystem is mounted.
>>
>> The change is described below:
>> a) Add a users count to nbd_device structure.
>> b) Add a bit flag to nbd_device structure of unsigned long.
>>
>> If the current user count is not 1 then make nbd-client wait
>> for the in_use bit to be cleared.
>
> Thanks, I like this approach much more.
>
>>
>> Signed-off-by: Pranay Kr. Srivastava <pranjas@xxxxxxxxx>
>> ---
>>  drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/nbd.h |  1 +
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 482a3c0..9b024d8 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -59,6 +59,7 @@ struct nbd_device {
>>       int xmit_timeout;
>>       atomic_t timedout;
>>       bool disconnect; /* a disconnect has been requested by user */
>> +     u32 users;
>
> Perhaps it is better to use kref for this?

Yup will do.

>
>>
>>       struct timer_list timeout_timer;
>>       /* protects initialization and shutdown of the socket */
>> @@ -69,6 +70,7 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>       struct dentry *dbg_dir;
>>  #endif
>> +     unsigned long bflags;   /* word size bit flags for use. */
>
> Maybe it is better to use a completion instead of a bitfield.

Ok.

>
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>               sock_shutdown(nbd);
>>               mutex_lock(&nbd->tx_lock);
>>               nbd_clear_que(nbd);
>> +             /*
>> +              * Wait for any users currently using
>> +              * this block device.
>> +              */
>> +             mutex_unlock(&nbd->tx_lock);
>> +             pr_info("Waiting for users to release device %s ...\n",
>> +                                             bdev->bd_disk->disk_name);
>> +             wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
>> +             mutex_lock(&nbd->tx_lock);
>>               kill_bdev(bdev);
>>               nbd_bdev_reset(bdev);
>>
>> @@ -870,10 +881,39 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>>       return error;
>>  }
>>
>> +static int nbd_open(struct block_device *bdev, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
>
> Here is a new line missing otherwise checkpatch will probably warn about
> this?
>
> Should we check here if we are connected here? And check whether the
> connection is about to be closed?

I don't think it should matter if we are connected or not. We already
handle that case
correctly. Requests would fail and those failures will be reported to
userland. The idea
here is not to enforce "harsh measures" on the user on failure but to
report them instead.

>
> Best Regards,
>
> Markus

I'll send in a new patch after making changes as per your review.

Thanks alot!.

>
>> +     nbd_dev->users++;
>> +     pr_debug("Opening nbd_dev %s. Active users = %u\n",
>> +                     bdev->bd_disk->disk_name, nbd_dev->users);
>> +     if (nbd_dev->users > 1)
>> +     {
>> +             set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
>> +     }
>> +     return 0;
>> +}
>> +
>> +static void nbd_release(struct gendisk *disk, fmode_t mode)
>> +{
>> +     struct nbd_device *nbd_dev = disk->private_data;
>> +     nbd_dev->users--;
>> +     pr_debug("Closing nbd_dev %s. Active users = %u\n",
>> +                     disk->disk_name, nbd_dev->users);
>> +     if (nbd_dev->users == 1)
>> +     {
>> +             clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
>> +             smp_mb();
>> +             wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
>> +     }
>> +}
>> +
>>  static const struct block_device_operations nbd_fops = {
>>       .owner =        THIS_MODULE,
>>       .ioctl =        nbd_ioctl,
>>       .compat_ioctl = nbd_ioctl,
>> +     .open =         nbd_open,
>> +     .release =      nbd_release
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
>> index e08e413..8f3d3f0 100644
>> --- a/include/uapi/linux/nbd.h
>> +++ b/include/uapi/linux/nbd.h
>> @@ -44,6 +44,7 @@ enum {
>>  /* there is a gap here to match userspace */
>>  #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
>>
>> +#define NBD_BFLAG_INUSE_BIT  (1) /* bit number for bflags */
>>  /* userspace doesn't need the nbd_device structure */
>>
>>  /* These are sent over the network in the request/reply magic fields */
>>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
        ---P.K.S