| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 May 2016 08:36:27 +0200
From: Markus Pargmann <mpa@...gutronix.de>
To: Pranay Srivastava <pranjas@...il.com>
Cc: nbd-general@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v4 18/18] make nbd device wait for its users in case of
timeout
Hi,
On Thu, May 12, 2016 at 08:49:00PM +0530, Pranay Srivastava wrote:
> On Thu, May 12, 2016 at 2:49 PM, Markus Pargmann <mpa@...gutronix.de> 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@...il.com>
> >> ---
> >> 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.
I see. Yes that's fine.
Thanks,
Markus
>
> >
> > 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
>
--
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 |
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists