Re: mountinfo contents changed when rootfs is ramfs

[Pranay Srivastava]

On Wed, Aug 19, 2020 at 5:20 PM Pranay Srivastava
 wrote:
>
> Hello,
>
> I'm running a system where rootfs is ramfs. For kernel version 5.2.11
>
> # cat /proc/self/mountinfo
> 0 0 0:1 / / rw - rootfs rootfs rw
> %<---snip>%
>
> while for kernel 5.4.58
> # cat /proc/self/mountinfo
> 0 0 0:1 / / rw - rootfs none rw
> %<---snip>%
>
> The reason for the above difference is because for kernel 5.2.11 the
> parse_param for
> rootfs was set to legacy_parse_param which handled the "source" param
> instead of
> ignoring it.
>
> With kernel 5.4.58 this is set to ramfs_parse_param which ignores any
> parameters not
> recognized and also returns 0 instead of -ENOPARAM. This causes
> vfs_parse_fs_param
> to not set the file context  source(fc->source) which results in "none" from
> alloc_vfs_mount(fc->source ? : "none")
>
> The commit which introduced the above change was
>
> commit f32356261d44d580649a7abce1156d15d49cf20f
> Author: David Howells 
> Date:   Mon Mar 25 16:38:31 2019 +
>
> vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount 
> API
>
> I'm not sure if this is a regression? But if it is, do we handle it like
>
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index ee179a81b3da..47a39baa0535 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
>
> @@ -200,7 +200,7 @@ static int ramfs_parse_param(struct fs_context
> *fc, struct fs_parameter *param)
>  * and as it is used as a !CONFIG_SHMEM simple substitute
>  * for tmpfs, better continue to ignore other mount options.
>  */
> -   if (opt == -ENOPARAM)
> +   if (opt == -ENOPARAM && strcmp(param->key, "source"))
> opt = 0;
> return opt;
>     }
>
> so that mountinfo gives the same information as for earlier kernels.
>
> Thanks!
> --
> Regards,
> Pranay Srivastava

Kindly let me know if the above is a regression and needs to be fixed?

-- 
Regards,
Pranay Srivastava

mountinfo contents changed when rootfs is ramfs

[Pranay Srivastava]

Hello,

I'm running a system where rootfs is ramfs. For kernel version 5.2.11

# cat /proc/self/mountinfo
0 0 0:1 / / rw - rootfs rootfs rw
%<---snip>%

while for kernel 5.4.58
# cat /proc/self/mountinfo
0 0 0:1 / / rw - rootfs none rw
%<---snip>%

The reason for the above difference is because for kernel 5.2.11 the
parse_param for
rootfs was set to legacy_parse_param which handled the "source" param
instead of
ignoring it.

With kernel 5.4.58 this is set to ramfs_parse_param which ignores any
parameters not
recognized and also returns 0 instead of -ENOPARAM. This causes
vfs_parse_fs_param
to not set the file context  source(fc->source) which results in "none" from
alloc_vfs_mount(fc->source ? : "none")

The commit which introduced the above change was

commit f32356261d44d580649a7abce1156d15d49cf20f
Author: David Howells 
Date:   Mon Mar 25 16:38:31 2019 +

vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API

I'm not sure if this is a regression? But if it is, do we handle it like

diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index ee179a81b3da..47a39baa0535 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c

@@ -200,7 +200,7 @@ static int ramfs_parse_param(struct fs_context
*fc, struct fs_parameter *param)
 * and as it is used as a !CONFIG_SHMEM simple substitute
 * for tmpfs, better continue to ignore other mount options.
 */
-   if (opt == -ENOPARAM)
+   if (opt == -ENOPARAM && strcmp(param->key, "source"))
opt = 0;
return opt;
}

so that mountinfo gives the same information as for earlier kernels.

Thanks!
-- 
Regards,
Pranay Srivastava

Re: [[RESEND]PATCH staging/speakup v3 3/3] use speakup_allocate as per required context

[Pranay Srivastava]

On Fri, Mar 24, 2017 at 2:13 PM, Greg KH  wrote:
> On Fri, Mar 24, 2017 at 02:07:11PM +0530, Pranay Kr. Srivastava wrote:
>> speakup_allocate used GFP_ATOMIC for allocations
>> even while during initialization due to it's use
>> in notifier call.
>
> Is that a problem?

No that's the way it should be. I was just trying to say that allocation
should be context based[?]. If we can be lenient then that's better[?]

>
>> Pass GFP_ flags as well to speakup_allocate depending
>> on the context it is called in.
>
> At init, we should be fine to use GFP_ATOMIC, so is this change really
> needed?
>
> thanks,
>
> greg k-h



-- 
---P.K.S

Re: [[RESEND]PATCH staging/speakup v3 3/3] use speakup_allocate as per required context

[Pranay Srivastava]

On Fri, Mar 24, 2017 at 2:13 PM, Greg KH  wrote:
> On Fri, Mar 24, 2017 at 02:07:11PM +0530, Pranay Kr. Srivastava wrote:
>> speakup_allocate used GFP_ATOMIC for allocations
>> even while during initialization due to it's use
>> in notifier call.
>
> Is that a problem?

No that's the way it should be. I was just trying to say that allocation
should be context based[?]. If we can be lenient then that's better[?]

>
>> Pass GFP_ flags as well to speakup_allocate depending
>> on the context it is called in.
>
> At init, we should be fine to use GFP_ATOMIC, so is this change really
> needed?
>
> thanks,
>
> greg k-h



-- 
---P.K.S

Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

On Sat, Jul 16, 2016 at 4:56 PM, Wouter Verhelst <w...@uter.be> wrote:
> On Sat, Jul 16, 2016 at 03:38:40PM +0530, Pranay Srivastava wrote:
>> Okay. So how about we include some negotiated key which goes in with every
>> request which the server could maintain for clients that can be checked while
>> resetting the connection with the same server?
>
> Wut?
>
>> So am I correct that this situation can
>> indeed happen or the server will throw an error back to client in case
>> the troubled
>> nbd-client is trying to reconnect to the original server but requests
>> are going to
>> another server?
>>
>> If yes to above query then what is the best effort we can do to avoid
>> such scenarios?
>
> Tell userspace not to do stupid things?
>
> This isn't a problem. The kernel assumes that whatever userspace does,
> once the connection is set up again everything's the way it was before.
> If that's not true, then userspace is to blame, not kernel space.
>
> Adding a "key" which we need to pass is going to make things wildly more
> complicated for no benefit.
Okay.
So let things roll for timeout but stop for disconnect.
>
> --
> < ron> I mean, the main *practical* problem with C++, is there's like a dozen
>people in the world who think they really understand all of its rules,
>and pretty much all of them are just lying to themselves too.
>  -- #debian-devel, OFTC, 2016-02-12



-- 
---P.K.S

Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

On Sat, Jul 16, 2016 at 4:56 PM, Wouter Verhelst  wrote:
> On Sat, Jul 16, 2016 at 03:38:40PM +0530, Pranay Srivastava wrote:
>> Okay. So how about we include some negotiated key which goes in with every
>> request which the server could maintain for clients that can be checked while
>> resetting the connection with the same server?
>
> Wut?
>
>> So am I correct that this situation can
>> indeed happen or the server will throw an error back to client in case
>> the troubled
>> nbd-client is trying to reconnect to the original server but requests
>> are going to
>> another server?
>>
>> If yes to above query then what is the best effort we can do to avoid
>> such scenarios?
>
> Tell userspace not to do stupid things?
>
> This isn't a problem. The kernel assumes that whatever userspace does,
> once the connection is set up again everything's the way it was before.
> If that's not true, then userspace is to blame, not kernel space.
>
> Adding a "key" which we need to pass is going to make things wildly more
> complicated for no benefit.
Okay.
So let things roll for timeout but stop for disconnect.
>
> --
> < ron> I mean, the main *practical* problem with C++, is there's like a dozen
>people in the world who think they really understand all of its rules,
>and pretty much all of them are just lying to themselves too.
>  -- #debian-devel, OFTC, 2016-02-12



-- 
---P.K.S

Re: [PATCH v5 3/4] make nbd device wait for its users

[Pranay Srivastava]

Hi Markus


On Sat, Jul 16, 2016 at 4:06 PM, Pranay Kr Srivastava  wrote:
> When a timeout occurs or a recv fails, then
> instead of abruptly killing nbd block device
> wait for its 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.
>
> Each open is now refcounted with the device being released
> for re-use only when there are no "other users".
>
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The reset happens
> when all tasks having this bdev open closes this bdev.
>
> Behavioral Change:
>
> 1)  NBD_DO_IT will not wait for the device to be reset. Hence
> the nbd-client "may exit" while some other process is using
> this device without actually doing the reset on this device
> , hence thus making it unusable until all such user space
> processes have stopped using this device.
>
> 2)  There's a window where the nbd-client will not be able to
> change / issue ioctls to the nbd device. This is when there's
> been a disconnect issued or a timeout has occured, however
> there are "other" user processes which currently have this
> nbd device opened.
>
> Signed-off-by: Pranay Kr Srivastava 
> ---
>  drivers/block/nbd.c | 37 -
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 4919760..fe36280 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -74,6 +74,7 @@ struct nbd_device {
>  *This is specifically for calling sock_shutdown, for now.
>  */
> struct work_struct ws_shutdown;
> +   atomic_t users; /* Users that opened the block device */
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -699,6 +700,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>unsigned int cmd, unsigned long arg)
>  {
> +   if (nbd->disconnect || nbd->timedout)
> +   return -EBUSY;
> +
> switch (cmd) {
> case NBD_DISCONNECT: {
> struct request sreq;
> @@ -728,7 +732,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> nbd_clear_que(nbd);
> BUG_ON(!list_empty(>queue_head));
> BUG_ON(!list_empty(>waiting_queue));
> -   kill_bdev(bdev);
> return 0;
>
> case NBD_SET_SOCK: {
> @@ -804,16 +807,12 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
> nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
>
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -852,10 +851,38 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +   struct block_device *bdev = bdget (part_devt(
> +   
> dev_to_part(nbd_to_dev(nbd;
> +
> +   WARN_ON(!bdev);
> +   if (atomic_dec_and_test(>users)) {
> +   if (bdev) {
> +   nbd_bdev_reset(bdev);
> +   kill_bdev(bdev);
> +   bdput(bdev);
> +   }
> +   nbd_reset(nbd);
> +   }
> +}
> +
>  static const struct block_device_operations nbd_fops = {
> .owner =THIS_MODULE,
> .ioctl =nbd_ioctl,
> .compat_ioctl = nbd_ioctl,
> +   .open = nbd_open,
> +   .release =  nbd_release,
>  };
>
>  static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> --
> 2.6.2
>
I've taken the patch you had send earlier , with atomics, and modified that.

I've added the ioctl handling part as part of this patch instead of keeping
it separate.

Let me know of any changes that should be done.
I'll send the whole series again later after taking the reviews of everyone.

-- 
---P.K.S

Re: [PATCH v5 3/4] make nbd device wait for its users

[Pranay Srivastava]

Hi Markus


On Sat, Jul 16, 2016 at 4:06 PM, Pranay Kr Srivastava  wrote:
> When a timeout occurs or a recv fails, then
> instead of abruptly killing nbd block device
> wait for its 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.
>
> Each open is now refcounted with the device being released
> for re-use only when there are no "other users".
>
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The reset happens
> when all tasks having this bdev open closes this bdev.
>
> Behavioral Change:
>
> 1)  NBD_DO_IT will not wait for the device to be reset. Hence
> the nbd-client "may exit" while some other process is using
> this device without actually doing the reset on this device
> , hence thus making it unusable until all such user space
> processes have stopped using this device.
>
> 2)  There's a window where the nbd-client will not be able to
> change / issue ioctls to the nbd device. This is when there's
> been a disconnect issued or a timeout has occured, however
> there are "other" user processes which currently have this
> nbd device opened.
>
> Signed-off-by: Pranay Kr Srivastava 
> ---
>  drivers/block/nbd.c | 37 -
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 4919760..fe36280 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -74,6 +74,7 @@ struct nbd_device {
>  *This is specifically for calling sock_shutdown, for now.
>  */
> struct work_struct ws_shutdown;
> +   atomic_t users; /* Users that opened the block device */
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -699,6 +700,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>unsigned int cmd, unsigned long arg)
>  {
> +   if (nbd->disconnect || nbd->timedout)
> +   return -EBUSY;
> +
> switch (cmd) {
> case NBD_DISCONNECT: {
> struct request sreq;
> @@ -728,7 +732,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> nbd_clear_que(nbd);
> BUG_ON(!list_empty(>queue_head));
> BUG_ON(!list_empty(>waiting_queue));
> -   kill_bdev(bdev);
> return 0;
>
> case NBD_SET_SOCK: {
> @@ -804,16 +807,12 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
> nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
>
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -852,10 +851,38 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +   struct block_device *bdev = bdget (part_devt(
> +   
> dev_to_part(nbd_to_dev(nbd;
> +
> +   WARN_ON(!bdev);
> +   if (atomic_dec_and_test(>users)) {
> +   if (bdev) {
> +   nbd_bdev_reset(bdev);
> +   kill_bdev(bdev);
> +   bdput(bdev);
> +   }
> +   nbd_reset(nbd);
> +   }
> +}
> +
>  static const struct block_device_operations nbd_fops = {
> .owner =THIS_MODULE,
> .ioctl =nbd_ioctl,
> .compat_ioctl = nbd_ioctl,
> +   .open = nbd_open,
> +   .release =  nbd_release,
>  };
>
>  static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> --
> 2.6.2
>
I've taken the patch you had send earlier , with atomics, and modified that.

I've added the ioctl handling part as part of this patch instead of keeping
it separate.

Let me know of any changes that should be done.
I'll send the whole series again later after taking the reviews of everyone.

-- 
---P.K.S

Re: [PATCH v5 2/4] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hi Markus,

On Sat, Jul 16, 2016 at 2:52 PM, Pranay Kr Srivastava  wrote:
> From: "Pranay Kr. Srivastava" 
>
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> v5)
>removed unnecessary code as per review of v4).
>
> Signed-off-by: Pranay Kr Srivastava 
> ---
>  drivers/block/nbd.c | 53 
> +++--
>  1 file changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 766c401..4919760 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,30 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -663,6 +664,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -798,10 +800,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
>
> +   sock_shutdown(nbd);
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
> -
> -   sock_shutdown(nbd);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
> @@ -857,6 +858,14 @@ static const struct block_device_operations nbd_fops = {
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> +{
> +   struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
> +   ws_shutdown);
> +
> +   sock_shutdown(nbd_dev);
> +}
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>
>  static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
> --
> 2.6.2
>

Made the changes you suggested. Let me know if these look alright so I
can resend
the whole series.


-- 
---P.K.S

Re: [PATCH v5 2/4] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hi Markus,

On Sat, Jul 16, 2016 at 2:52 PM, Pranay Kr Srivastava  wrote:
> From: "Pranay Kr. Srivastava" 
>
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> v5)
>removed unnecessary code as per review of v4).
>
> Signed-off-by: Pranay Kr Srivastava 
> ---
>  drivers/block/nbd.c | 53 
> +++--
>  1 file changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 766c401..4919760 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,30 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -663,6 +664,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -798,10 +800,9 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
>
> +   sock_shutdown(nbd);
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
> -
> -   sock_shutdown(nbd);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
> @@ -857,6 +858,14 @@ static const struct block_device_operations nbd_fops = {
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> +{
> +   struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
> +   ws_shutdown);
> +
> +   sock_shutdown(nbd_dev);
> +}
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>
>  static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
> --
> 2.6.2
>

Made the changes you suggested. Let me know if these look alright so I
can resend
the whole series.


-- 
---P.K.S

Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

On Sat, Jul 16, 2016 at 3:02 PM, Alex Bligh <a...@alex.org.uk> wrote:
>
> On 16 Jul 2016, at 08:42, Pranay Srivastava <pran...@gmail.com> wrote:
>
>> So instead can't we put a mechanism in place for network address + mac
>> to be same
>> for allowing clients to reconnect? Do let me know if this is not of concern.
>
> MAC address?! nbd clients connect over IP, and if a router reboots
> between them, you could easily see two packets from the same client
> come from different MAC addresses. Similarly all clients not on
> the same L2 network will carry the same MAC address. So MAC address
> is a very poor indicator of 'same client'.
>
> IP address is also a poor indicator (think NAT) but is substantially
> less bad.

Okay. So how about we include some negotiated key which goes in with every
request which the server could maintain for clients that can be checked while
resetting the connection with the same server?

So am I correct that this situation can
indeed happen or the server will throw an error back to client in case
the troubled
nbd-client is trying to reconnect to the original server but requests
are going to
another server?

If yes to above query then what is the best effort we can do to avoid
such scenarios?

>
> --
> Alex Bligh
>
>
>
>



-- 
---P.K.S

Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

On Sat, Jul 16, 2016 at 3:02 PM, Alex Bligh  wrote:
>
> On 16 Jul 2016, at 08:42, Pranay Srivastava  wrote:
>
>> So instead can't we put a mechanism in place for network address + mac
>> to be same
>> for allowing clients to reconnect? Do let me know if this is not of concern.
>
> MAC address?! nbd clients connect over IP, and if a router reboots
> between them, you could easily see two packets from the same client
> come from different MAC addresses. Similarly all clients not on
> the same L2 network will carry the same MAC address. So MAC address
> is a very poor indicator of 'same client'.
>
> IP address is also a poor indicator (think NAT) but is substantially
> less bad.

Okay. So how about we include some negotiated key which goes in with every
request which the server could maintain for clients that can be checked while
resetting the connection with the same server?

So am I correct that this situation can
indeed happen or the server will throw an error back to client in case
the troubled
nbd-client is trying to reconnect to the original server but requests
are going to
another server?

If yes to above query then what is the best effort we can do to avoid
such scenarios?

>
> --
> Alex Bligh
>
>
>
>



-- 
---P.K.S

Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

Hi,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> After NBD_DO_IT exited the block device may still be used. Make sure
> that we handle intended disconnects differently and do not allow any
> changed of the nbd device.
>
> This patch should avoid that the nbd-client connects to a different server
> and the users of the block device are suddenly reading/writing from a
> different backend device.
>
> For timeouts it is still possible to setup a new socket so that the
> connection may be refreshed without creating problems for all users.

But Shouldn't time out be checked for last end point?

For example, consider the following steps

1) Timeout occurs but server[nbd-s1] comes up again albeit with a different
network address.

2) The previous network address of server [nbd-s1] has now been assigned to
another new nbd server [nbd-s2]

3) A new nbd-client tries to setup the socket again, Negotiation would
be done again
[correct?]. If correct then wouldn't we be sending data to wrong
device this time?

So instead can't we put a mechanism in place for network address + mac
to be same
for allowing clients to reconnect? Do let me know if this is not of concern.

4) If 3) doesn't apply then let's disallow all ioctls until nbd device
is reset.

>
> Signed-off-by: Markus Pargmann 
> ---
>  drivers/block/nbd.c | 30 --
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 620660f3ff0f..39358efac73e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -708,6 +708,18 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>unsigned int cmd, unsigned long arg)
>  {
> +   /*
> +* After a disconnect was instructed, do not allow any further actions
> +* on the block device that would lead to a new connected endpoint.
> +* This condition stays until nbd_reset was called either because all
> +* users closed the device or because of CLEAR_SOCK.
> +*/
> +   if (nbd->disconnect &&
> +   cmd != NBD_CLEAR_SOCK && cmd != NBD_PRINT_DEBUG) {
> +   dev_info(disk_to_dev(nbd->disk), "Device is still busy after 
> instructing a disconnect\n");
> +   return -EBUSY;
> +   }
> +
> switch (cmd) {
> case NBD_DISCONNECT: {
> struct request sreq;
> @@ -733,11 +745,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> }
>
> case NBD_CLEAR_SOCK:
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   BUG_ON(!list_empty(>queue_head));
> -   BUG_ON(!list_empty(>waiting_queue));
> -   kill_bdev(bdev);
> +   if (nbd->disconnect) {
> +   nbd_reset(nbd);
> +   } else {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   BUG_ON(!list_empty(>queue_head));
> +   BUG_ON(!list_empty(>waiting_queue));
> +   kill_bdev(bdev);
> +   }
> return 0;
>
> case NBD_SET_SOCK: {
> @@ -812,8 +828,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> +   if (nbd->disconnect) { /* user requested, ignore socket 
> errors */
> +   sock_shutdown(nbd);
> error = 0;
> +   }
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> --
> 2.1.4
>



-- 
---P.K.S

Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

Hi,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> After NBD_DO_IT exited the block device may still be used. Make sure
> that we handle intended disconnects differently and do not allow any
> changed of the nbd device.
>
> This patch should avoid that the nbd-client connects to a different server
> and the users of the block device are suddenly reading/writing from a
> different backend device.
>
> For timeouts it is still possible to setup a new socket so that the
> connection may be refreshed without creating problems for all users.

But Shouldn't time out be checked for last end point?

For example, consider the following steps

1) Timeout occurs but server[nbd-s1] comes up again albeit with a different
network address.

2) The previous network address of server [nbd-s1] has now been assigned to
another new nbd server [nbd-s2]

3) A new nbd-client tries to setup the socket again, Negotiation would
be done again
[correct?]. If correct then wouldn't we be sending data to wrong
device this time?

So instead can't we put a mechanism in place for network address + mac
to be same
for allowing clients to reconnect? Do let me know if this is not of concern.

4) If 3) doesn't apply then let's disallow all ioctls until nbd device
is reset.

>
> Signed-off-by: Markus Pargmann 
> ---
>  drivers/block/nbd.c | 30 --
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 620660f3ff0f..39358efac73e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -708,6 +708,18 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>unsigned int cmd, unsigned long arg)
>  {
> +   /*
> +* After a disconnect was instructed, do not allow any further actions
> +* on the block device that would lead to a new connected endpoint.
> +* This condition stays until nbd_reset was called either because all
> +* users closed the device or because of CLEAR_SOCK.
> +*/
> +   if (nbd->disconnect &&
> +   cmd != NBD_CLEAR_SOCK && cmd != NBD_PRINT_DEBUG) {
> +   dev_info(disk_to_dev(nbd->disk), "Device is still busy after 
> instructing a disconnect\n");
> +   return -EBUSY;
> +   }
> +
> switch (cmd) {
> case NBD_DISCONNECT: {
> struct request sreq;
> @@ -733,11 +745,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> }
>
> case NBD_CLEAR_SOCK:
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   BUG_ON(!list_empty(>queue_head));
> -   BUG_ON(!list_empty(>waiting_queue));
> -   kill_bdev(bdev);
> +   if (nbd->disconnect) {
> +   nbd_reset(nbd);
> +   } else {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   BUG_ON(!list_empty(>queue_head));
> +   BUG_ON(!list_empty(>waiting_queue));
> +   kill_bdev(bdev);
> +   }
> return 0;
>
> case NBD_SET_SOCK: {
> @@ -812,8 +828,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
> nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> +   if (nbd->disconnect) { /* user requested, ignore socket 
> errors */
> +   sock_shutdown(nbd);
> error = 0;
> +   }
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> --
> 2.1.4
>



-- 
---P.K.S

Re: [PATCH v4 2/5]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Wed, Jul 13, 2016 at 12:43 PM, Markus Pargmann <m...@pengutronix.de> wrote:
> On Sunday 10 July 2016 21:03:05 Pranay Srivastava wrote:
>> On Sunday, July 10, 2016, Markus Pargmann <m...@pengutronix.de> wrote:
>> > Hi,
>> >
>> > On 2016 M06 30, Thu 14:02:02 CEST Pranay Kr. Srivastava wrote:
>> >> spinlocked ranges should be small and not contain calls into huge
>> >> subfunctions. Fix my mistake and just get the pointer to the socket
>> >> instead of doing everything with spinlock held.
>> >>
>> >> Reported-by: Mikulas Patocka <miku...@twibright.com>
>> >> Signed-off-by: Markus Pargmann <m...@pengutronix.de>
>> >>
>> >> Changelog:
>> >> Pranay Kr. Srivastava<pran...@gmail.com>:
>> >>
>> >> 1) Use spin_lock instead of irq version for sock_shutdown.
>> >>
>> >> 2) Use system work queue to actually trigger the shutdown of
>> >> socket. This solves the issue when kernel_sendmsg is currently
>> >> blocked while a timeout occurs.
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>> >> ---
>> >>  drivers/block/nbd.c | 57
>> >> + 1 file changed, 36
>> >> insertions(+), 21 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index 766c401..e362d44 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -39,6 +39,7 @@
>> >>  #include 
>> >>
>> >>  #include 
>> >> +#include 
>> >>
>> >>  struct nbd_device {
>> >>   u32 flags;
>> >> @@ -69,6 +70,8 @@ struct nbd_device {
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >>   struct dentry *dbg_dir;
>> >>  #endif
>> >> + /* This is specifically for calling sock_shutdown, for now. */
>> >> + struct work_struct ws_shutdown;
>> >>  };
>> >>
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >> @@ -95,6 +98,8 @@ static int max_part;
>> >>   */
>> >>  static DEFINE_SPINLOCK(nbd_lock);
>> >>
>> >> +static void nbd_ws_func_shutdown(struct work_struct *);
>> >> +
>> >
>> > are you reading all the comments I had?...
>> >
>> > At least respond to my comments if you disagree. I still can't see the
>> benefit
>> > of a function signature here if we can avoid it.
>> >
>>
>> That would require some code to be moved. So to avoid those
>> unnecessary changes it was better to have a prototype.
>>
>> It would've pissed you off more if I had tried
>> to get rid of protoype.
>
> Ah I see, thanks.
>
>>
>> >>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> >>  {
>> >>   return disk_to_dev(nbd->disk);
>> >> @@ -172,39 +177,36 @@ static void nbd_end_request(struct nbd_device *nbd,
>> >> struct request *req) */
>> >>  static void sock_shutdown(struct nbd_device *nbd)
>> >>  {
>> >> - spin_lock_irq(>sock_lock);
>> >> -
>> >> - if (!nbd->sock) {
>> >> - spin_unlock_irq(>sock_lock);
>> >> - return;
>> >> - }
>> >> + struct socket *sock;
>> >>
>> >> - dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> >> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> >> - sockfd_put(nbd->sock);
>> >> + spin_lock(>sock_lock);
>> >> + sock = nbd->sock;
>> >>   nbd->sock = NULL;
>> >> - spin_unlock_irq(>sock_lock);
>> >> + spin_unlock(>sock_lock);
>> >> +
>> >> + if (!sock)
>> >> + return;
>> >>
>> >>   del_timer(>timeout_timer);
>> >> + dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> >> + kernel_sock_shutdown(sock, SHUT_RDWR);
>> >> + sockfd_put(sock);
>> >>  }
>> >>
>> >>  static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >> - unsigned long flags;
>> >>
>> >>   if (list_em

Re: [PATCH v4 2/5]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Wed, Jul 13, 2016 at 12:43 PM, Markus Pargmann  wrote:
> On Sunday 10 July 2016 21:03:05 Pranay Srivastava wrote:
>> On Sunday, July 10, 2016, Markus Pargmann  wrote:
>> > Hi,
>> >
>> > On 2016 M06 30, Thu 14:02:02 CEST Pranay Kr. Srivastava wrote:
>> >> spinlocked ranges should be small and not contain calls into huge
>> >> subfunctions. Fix my mistake and just get the pointer to the socket
>> >> instead of doing everything with spinlock held.
>> >>
>> >> Reported-by: Mikulas Patocka 
>> >> Signed-off-by: Markus Pargmann 
>> >>
>> >> Changelog:
>> >> Pranay Kr. Srivastava:
>> >>
>> >> 1) Use spin_lock instead of irq version for sock_shutdown.
>> >>
>> >> 2) Use system work queue to actually trigger the shutdown of
>> >> socket. This solves the issue when kernel_sendmsg is currently
>> >> blocked while a timeout occurs.
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava 
>> >> ---
>> >>  drivers/block/nbd.c | 57
>> >> + 1 file changed, 36
>> >> insertions(+), 21 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index 766c401..e362d44 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -39,6 +39,7 @@
>> >>  #include 
>> >>
>> >>  #include 
>> >> +#include 
>> >>
>> >>  struct nbd_device {
>> >>   u32 flags;
>> >> @@ -69,6 +70,8 @@ struct nbd_device {
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >>   struct dentry *dbg_dir;
>> >>  #endif
>> >> + /* This is specifically for calling sock_shutdown, for now. */
>> >> + struct work_struct ws_shutdown;
>> >>  };
>> >>
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >> @@ -95,6 +98,8 @@ static int max_part;
>> >>   */
>> >>  static DEFINE_SPINLOCK(nbd_lock);
>> >>
>> >> +static void nbd_ws_func_shutdown(struct work_struct *);
>> >> +
>> >
>> > are you reading all the comments I had?...
>> >
>> > At least respond to my comments if you disagree. I still can't see the
>> benefit
>> > of a function signature here if we can avoid it.
>> >
>>
>> That would require some code to be moved. So to avoid those
>> unnecessary changes it was better to have a prototype.
>>
>> It would've pissed you off more if I had tried
>> to get rid of protoype.
>
> Ah I see, thanks.
>
>>
>> >>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> >>  {
>> >>   return disk_to_dev(nbd->disk);
>> >> @@ -172,39 +177,36 @@ static void nbd_end_request(struct nbd_device *nbd,
>> >> struct request *req) */
>> >>  static void sock_shutdown(struct nbd_device *nbd)
>> >>  {
>> >> - spin_lock_irq(>sock_lock);
>> >> -
>> >> - if (!nbd->sock) {
>> >> - spin_unlock_irq(>sock_lock);
>> >> - return;
>> >> - }
>> >> + struct socket *sock;
>> >>
>> >> - dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> >> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> >> - sockfd_put(nbd->sock);
>> >> + spin_lock(>sock_lock);
>> >> + sock = nbd->sock;
>> >>   nbd->sock = NULL;
>> >> - spin_unlock_irq(>sock_lock);
>> >> + spin_unlock(>sock_lock);
>> >> +
>> >> + if (!sock)
>> >> + return;
>> >>
>> >>   del_timer(>timeout_timer);
>> >> + dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> >> + kernel_sock_shutdown(sock, SHUT_RDWR);
>> >> + sockfd_put(sock);
>> >>  }
>> >>
>> >>  static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >> - unsigned long flags;
>> >>
>> >>   if (list_empty(>queue_head))
>> >>   return;
>> >>
>> >> - spin_lock_irqsave(>sock_lock, flags);
>> >> -

Re: [PATCH v4 3/5]nbd: make nbd device wait for its users

[Pranay Srivastava]

On Wed, Jul 13, 2016 at 1:24 PM, Markus Pargmann <m...@pengutronix.de> wrote:
> On Sunday 10 July 2016 21:32:07 Pranay Srivastava wrote:
>> On Sun, Jul 10, 2016 at 6:32 PM, Markus Pargmann <m...@pengutronix.de> wrote:
>> > On 2016 M06 30, Thu 14:02:03 CEST Pranay Kr. Srivastava wrote:
>> >> When a timeout occurs or a recv fails, then
>> >> instead of abruplty killing nbd block device
>> >> wait for its 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.
>> >>
>> >> Each open of a nbd device is refcounted, while
>> >> the userland program [nbd-client] doing the
>> >> NBD_DO_IT ioctl would now wait for any other users
>> >> of this device before invalidating the nbd device.
>> >>
>> >> A timedout or a disconnected device, if in use, can't
>> >> be used until it has been resetted. The reset happens
>> >> when all tasks having this bdev open closes this bdev.
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>> >> ---
>> >>  drivers/block/nbd.c | 106
>> >> ++-- 1 file changed, 87
>> >> insertions(+), 19 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index e362d44..fb56dd2 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -72,6 +72,8 @@ struct nbd_device {
>> >>  #endif
>> >>   /* This is specifically for calling sock_shutdown, for now. */
>> >>   struct work_struct ws_shutdown;
>> >> + struct kref users;
>> >> + struct completion user_completion;
>> >>  };
>> >>
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >> @@ -99,6 +101,8 @@ static int max_part;
>> >>  static DEFINE_SPINLOCK(nbd_lock);
>> >>
>> >>  static void nbd_ws_func_shutdown(struct work_struct *);
>> >> +static void nbd_kref_release(struct kref *);
>> >> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>> >
>> > More function signatures. Why?
>>
>> To avoid code move. But do let me know why is code signature(s)
>> like this are bad , just asking to avoid such things.
>>
>> >
>> >>
>> >>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> >>  {
>> >> @@ -145,11 +149,9 @@ static int nbd_size_set(struct nbd_device *nbd, 
>> >> struct
>> >> block_device *bdev, int blocksize, int nr_blocks)
>> >>  {
>> >>   int ret;
>> >> -
>> >>   ret = set_blocksize(bdev, blocksize);
>> >>   if (ret)
>> >>   return ret;
>> >> -
>> >
>> > Unrelated.
>> >
>> >>   nbd->blksize = blocksize;
>> >>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>> >>
>> >> @@ -197,6 +199,9 @@ static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >>
>> >> + if (nbd->timedout)
>> >> + return;
>> >> +
>> >
>> > What does this have to do with the patch?
>>
>> to avoid re-scheduling the work function. Apparently that did
>> cause some trouble with ext4 and 10K dd processes.
>
> Ah interesting. What was the timeout in this scenario?

Not much about 5 or 6 secs. The client was on a VM though on my laptop.
I think it was due to disconnect being set and then kill_bdev called multiple
times. I didn't explored this much as doing the check for this solved the
problem.

I also sent a patch for ext4 as well as that also caused a BUG to be triggered
in fs/buffer.c while the buffer was being marked dirty and in parallel the same
buffer reported an EIO.

>
>>
>> >
>> >>   if (list_empty(>queue_head))
>> >>   return;
>> >>
>> >> @@ -472,8 +477,6 @@ static int nbd_thread_recv(struct nbd_device *nbd,
>> >> struct block_device *bdev) nbd_end_request(nbd, req);
>> >>   }
>> >>
>> >> - nbd_size_clear(nbd, bdev);
>> >> -
>> >>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
&g

Re: [PATCH v4 3/5]nbd: make nbd device wait for its users

[Pranay Srivastava]

On Wed, Jul 13, 2016 at 1:24 PM, Markus Pargmann  wrote:
> On Sunday 10 July 2016 21:32:07 Pranay Srivastava wrote:
>> On Sun, Jul 10, 2016 at 6:32 PM, Markus Pargmann  wrote:
>> > On 2016 M06 30, Thu 14:02:03 CEST Pranay Kr. Srivastava wrote:
>> >> When a timeout occurs or a recv fails, then
>> >> instead of abruplty killing nbd block device
>> >> wait for its 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.
>> >>
>> >> Each open of a nbd device is refcounted, while
>> >> the userland program [nbd-client] doing the
>> >> NBD_DO_IT ioctl would now wait for any other users
>> >> of this device before invalidating the nbd device.
>> >>
>> >> A timedout or a disconnected device, if in use, can't
>> >> be used until it has been resetted. The reset happens
>> >> when all tasks having this bdev open closes this bdev.
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava 
>> >> ---
>> >>  drivers/block/nbd.c | 106
>> >> ++-- 1 file changed, 87
>> >> insertions(+), 19 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index e362d44..fb56dd2 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -72,6 +72,8 @@ struct nbd_device {
>> >>  #endif
>> >>   /* This is specifically for calling sock_shutdown, for now. */
>> >>   struct work_struct ws_shutdown;
>> >> + struct kref users;
>> >> + struct completion user_completion;
>> >>  };
>> >>
>> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> >> @@ -99,6 +101,8 @@ static int max_part;
>> >>  static DEFINE_SPINLOCK(nbd_lock);
>> >>
>> >>  static void nbd_ws_func_shutdown(struct work_struct *);
>> >> +static void nbd_kref_release(struct kref *);
>> >> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>> >
>> > More function signatures. Why?
>>
>> To avoid code move. But do let me know why is code signature(s)
>> like this are bad , just asking to avoid such things.
>>
>> >
>> >>
>> >>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> >>  {
>> >> @@ -145,11 +149,9 @@ static int nbd_size_set(struct nbd_device *nbd, 
>> >> struct
>> >> block_device *bdev, int blocksize, int nr_blocks)
>> >>  {
>> >>   int ret;
>> >> -
>> >>   ret = set_blocksize(bdev, blocksize);
>> >>   if (ret)
>> >>   return ret;
>> >> -
>> >
>> > Unrelated.
>> >
>> >>   nbd->blksize = blocksize;
>> >>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>> >>
>> >> @@ -197,6 +199,9 @@ static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >>
>> >> + if (nbd->timedout)
>> >> + return;
>> >> +
>> >
>> > What does this have to do with the patch?
>>
>> to avoid re-scheduling the work function. Apparently that did
>> cause some trouble with ext4 and 10K dd processes.
>
> Ah interesting. What was the timeout in this scenario?

Not much about 5 or 6 secs. The client was on a VM though on my laptop.
I think it was due to disconnect being set and then kill_bdev called multiple
times. I didn't explored this much as doing the check for this solved the
problem.

I also sent a patch for ext4 as well as that also caused a BUG to be triggered
in fs/buffer.c while the buffer was being marked dirty and in parallel the same
buffer reported an EIO.

>
>>
>> >
>> >>   if (list_empty(>queue_head))
>> >>   return;
>> >>
>> >> @@ -472,8 +477,6 @@ static int nbd_thread_recv(struct nbd_device *nbd,
>> >> struct block_device *bdev) nbd_end_request(nbd, req);
>> >>   }
>> >>
>> >> - nbd_size_clear(nbd, bdev);
>> >> -
>> >>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>> >>
>> >>   nbd->task_recv = NULL;
>> >&

Re: [PATCH v4 3/5]nbd: make nbd device wait for its users

[Pranay Srivastava]

On Sun, Jul 10, 2016 at 6:32 PM, Markus Pargmann  wrote:
> On 2016 M06 30, Thu 14:02:03 CEST Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>> wait for its 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.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The reset happens
>> when all tasks having this bdev open closes this bdev.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 106
>> ++-- 1 file changed, 87
>> insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index e362d44..fb56dd2 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -72,6 +72,8 @@ struct nbd_device {
>>  #endif
>>   /* This is specifically for calling sock_shutdown, for now. */
>>   struct work_struct ws_shutdown;
>> + struct kref users;
>> + struct completion user_completion;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -99,6 +101,8 @@ static int max_part;
>>  static DEFINE_SPINLOCK(nbd_lock);
>>
>>  static void nbd_ws_func_shutdown(struct work_struct *);
>> +static void nbd_kref_release(struct kref *);
>> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>
> More function signatures. Why?

To avoid code move. But do let me know why is code signature(s)
like this are bad , just asking to avoid such things.

>
>>
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>> @@ -145,11 +149,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct
>> block_device *bdev, int blocksize, int nr_blocks)
>>  {
>>   int ret;
>> -
>>   ret = set_blocksize(bdev, blocksize);
>>   if (ret)
>>   return ret;
>> -
>
> Unrelated.
>
>>   nbd->blksize = blocksize;
>>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>>
>> @@ -197,6 +199,9 @@ static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>>
>> + if (nbd->timedout)
>> + return;
>> +
>
> What does this have to do with the patch?

to avoid re-scheduling the work function. Apparently that did
cause some trouble with ext4 and 10K dd processes.

>
>>   if (list_empty(>queue_head))
>>   return;
>>
>> @@ -472,8 +477,6 @@ static int nbd_thread_recv(struct nbd_device *nbd,
>> struct block_device *bdev) nbd_end_request(nbd, req);
>>   }
>>
>> - nbd_size_clear(nbd, bdev);
>> -
>>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>>
>>   nbd->task_recv = NULL;
>> @@ -650,12 +653,13 @@ static int nbd_set_socket(struct nbd_device *nbd,
>> struct socket *sock) int ret = 0;
>>
>>   spin_lock(>sock_lock);
>> - if (nbd->sock)
>> +
>> + if (nbd->sock || nbd->timedout)
>>   ret = -EBUSY;
>
> nbd->timedout is already checked in __nbd_ioctl(), no need to check it twice.
>
>>   else
>>   nbd->sock = sock;
>> - spin_unlock(>sock_lock);
>>
>> + spin_unlock(>sock_lock);
>
> random modification.
>
>>   return ret;
>>  }
>>
>> @@ -670,6 +674,7 @@ static void nbd_reset(struct nbd_device *nbd)
>>   nbd->flags = 0;
>>   nbd->xmit_timeout = 0;
>>   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>> + init_completion(>user_completion);
>>   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>>   del_timer_sync(>timeout_timer);
>>  }
>> @@ -704,6 +709,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>  unsigned int cmd, unsigned long arg)
>>  {
>> + if (nbd->timedout || nbd->disconnect)
>> + return -EBUSY;
>> +
>>   switch (cmd) {
>>   case NBD_DISCONNECT: {
>>   struct request sreq;
>> @@ -733,7 +741,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct
>> nbd_device *nbd, nbd_clear_que(nbd);
>>   BUG_ON(!list_empty(>queue_head));
>>   BUG_ON(!list_empty(>waiting_queue));
>> - kill_bdev(bdev);
>>   return 0;
>>
>>   case NBD_SET_SOCK: {
>> @@ -752,7 +759,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct
>> nbd_device *nbd,
>>
>>   case NBD_SET_BLKSIZE: {
>>   loff_t bsize = div_s64(nbd->bytesize, arg);
>> -
>
> random modification.
>
>>   return nbd_size_set(nbd, bdev, arg, bsize);
>>   }
>>
>> @@ -804,22 +810,29 @@ static int __nbd_ioctl(struct block_device *bdev,
>> 

Re: [PATCH v4 3/5]nbd: make nbd device wait for its users

[Pranay Srivastava]

On Sun, Jul 10, 2016 at 6:32 PM, Markus Pargmann  wrote:
> On 2016 M06 30, Thu 14:02:03 CEST Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>> wait for its 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.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The reset happens
>> when all tasks having this bdev open closes this bdev.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 106
>> ++-- 1 file changed, 87
>> insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index e362d44..fb56dd2 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -72,6 +72,8 @@ struct nbd_device {
>>  #endif
>>   /* This is specifically for calling sock_shutdown, for now. */
>>   struct work_struct ws_shutdown;
>> + struct kref users;
>> + struct completion user_completion;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -99,6 +101,8 @@ static int max_part;
>>  static DEFINE_SPINLOCK(nbd_lock);
>>
>>  static void nbd_ws_func_shutdown(struct work_struct *);
>> +static void nbd_kref_release(struct kref *);
>> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>
> More function signatures. Why?

To avoid code move. But do let me know why is code signature(s)
like this are bad , just asking to avoid such things.

>
>>
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>> @@ -145,11 +149,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct
>> block_device *bdev, int blocksize, int nr_blocks)
>>  {
>>   int ret;
>> -
>>   ret = set_blocksize(bdev, blocksize);
>>   if (ret)
>>   return ret;
>> -
>
> Unrelated.
>
>>   nbd->blksize = blocksize;
>>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>>
>> @@ -197,6 +199,9 @@ static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>>
>> + if (nbd->timedout)
>> + return;
>> +
>
> What does this have to do with the patch?

to avoid re-scheduling the work function. Apparently that did
cause some trouble with ext4 and 10K dd processes.

>
>>   if (list_empty(>queue_head))
>>   return;
>>
>> @@ -472,8 +477,6 @@ static int nbd_thread_recv(struct nbd_device *nbd,
>> struct block_device *bdev) nbd_end_request(nbd, req);
>>   }
>>
>> - nbd_size_clear(nbd, bdev);
>> -
>>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>>
>>   nbd->task_recv = NULL;
>> @@ -650,12 +653,13 @@ static int nbd_set_socket(struct nbd_device *nbd,
>> struct socket *sock) int ret = 0;
>>
>>   spin_lock(>sock_lock);
>> - if (nbd->sock)
>> +
>> + if (nbd->sock || nbd->timedout)
>>   ret = -EBUSY;
>
> nbd->timedout is already checked in __nbd_ioctl(), no need to check it twice.
>
>>   else
>>   nbd->sock = sock;
>> - spin_unlock(>sock_lock);
>>
>> + spin_unlock(>sock_lock);
>
> random modification.
>
>>   return ret;
>>  }
>>
>> @@ -670,6 +674,7 @@ static void nbd_reset(struct nbd_device *nbd)
>>   nbd->flags = 0;
>>   nbd->xmit_timeout = 0;
>>   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>> + init_completion(>user_completion);
>>   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>>   del_timer_sync(>timeout_timer);
>>  }
>> @@ -704,6 +709,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>>  unsigned int cmd, unsigned long arg)
>>  {
>> + if (nbd->timedout || nbd->disconnect)
>> + return -EBUSY;
>> +
>>   switch (cmd) {
>>   case NBD_DISCONNECT: {
>>   struct request sreq;
>> @@ -733,7 +741,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct
>> nbd_device *nbd, nbd_clear_que(nbd);
>>   BUG_ON(!list_empty(>queue_head));
>>   BUG_ON(!list_empty(>waiting_queue));
>> - kill_bdev(bdev);
>>   return 0;
>>
>>   case NBD_SET_SOCK: {
>> @@ -752,7 +759,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct
>> nbd_device *nbd,
>>
>>   case NBD_SET_BLKSIZE: {
>>   loff_t bsize = div_s64(nbd->bytesize, arg);
>> -
>
> random modification.
>
>>   return nbd_size_set(nbd, bdev, arg, bsize);
>>   }
>>
>> @@ -804,22 +810,29 @@ static int __nbd_ioctl(struct block_device *bdev,
>> struct nbd_device *nbd, error = 

Re: [PATCH v4 1/5]nbd: cleanup nbd_set_socket

[Pranay Srivastava]

On Thu, Jul 7, 2016 at 8:26 PM, Pranay Srivastava <pran...@gmail.com> wrote:
> On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
> <pran...@gmail.com> wrote:
>> From: "Pranay Kr. Srivastava" <pranaykumar.srivast...@cognizant.com>
>>
>> This patch
>> 1) uses spin_lock instead of irq version.
>>
>> 2) removes the goto statement in case a socket
>>is already assigned with simple if-else statement.
>>
>> Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>> ---
>>  drivers/block/nbd.c | 15 +--
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 56f7f5d..766c401 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -643,17 +643,12 @@ static int nbd_set_socket(struct nbd_device *nbd, 
>> struct socket *sock)
>>  {
>> int ret = 0;
>>
>> -   spin_lock_irq(>sock_lock);
>> -
>> -   if (nbd->sock) {
>> +   spin_lock(>sock_lock);
>> +   if (nbd->sock)
>> ret = -EBUSY;
>> -   goto out;
>> -   }
>> -
>> -   nbd->sock = sock;
>> -
>> -out:
>> -   spin_unlock_irq(>sock_lock);
>> +   else
>> +   nbd->sock = sock;
>> +   spin_unlock(>sock_lock);
>>
>> return ret;
>>  }
>> --
>> 1.9.1
>>
>
> Hi Markus,
>
> Did you get a chance to review V4 of this series.
>
> --
> ---P.K.S

Ping ?

-- 
---P.K.S

Re: [PATCH v4 1/5]nbd: cleanup nbd_set_socket

[Pranay Srivastava]

On Thu, Jul 7, 2016 at 8:26 PM, Pranay Srivastava  wrote:
> On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
>  wrote:
>> From: "Pranay Kr. Srivastava" 
>>
>> This patch
>> 1) uses spin_lock instead of irq version.
>>
>> 2) removes the goto statement in case a socket
>>is already assigned with simple if-else statement.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 15 +--
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 56f7f5d..766c401 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -643,17 +643,12 @@ static int nbd_set_socket(struct nbd_device *nbd, 
>> struct socket *sock)
>>  {
>> int ret = 0;
>>
>> -   spin_lock_irq(>sock_lock);
>> -
>> -   if (nbd->sock) {
>> +   spin_lock(>sock_lock);
>> +   if (nbd->sock)
>> ret = -EBUSY;
>> -   goto out;
>> -   }
>> -
>> -   nbd->sock = sock;
>> -
>> -out:
>> -   spin_unlock_irq(>sock_lock);
>> +   else
>> +   nbd->sock = sock;
>> +   spin_unlock(>sock_lock);
>>
>> return ret;
>>  }
>> --
>> 1.9.1
>>
>
> Hi Markus,
>
> Did you get a chance to review V4 of this series.
>
> --
> ---P.K.S

Ping ?

-- 
---P.K.S

Re: [PATCH v4 1/5]nbd: cleanup nbd_set_socket

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
 wrote:
> From: "Pranay Kr. Srivastava" 
>
> This patch
> 1) uses spin_lock instead of irq version.
>
> 2) removes the goto statement in case a socket
>is already assigned with simple if-else statement.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 15 +--
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 56f7f5d..766c401 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -643,17 +643,12 @@ static int nbd_set_socket(struct nbd_device *nbd, 
> struct socket *sock)
>  {
> int ret = 0;
>
> -   spin_lock_irq(>sock_lock);
> -
> -   if (nbd->sock) {
> +   spin_lock(>sock_lock);
> +   if (nbd->sock)
> ret = -EBUSY;
> -   goto out;
> -   }
> -
> -   nbd->sock = sock;
> -
> -out:
> -   spin_unlock_irq(>sock_lock);
> +   else
> +   nbd->sock = sock;
> +   spin_unlock(>sock_lock);
>
> return ret;
>  }
> --
> 1.9.1
>

Hi Markus,

Did you get a chance to review V4 of this series.

-- 
---P.K.S

Re: [PATCH v4 1/5]nbd: cleanup nbd_set_socket

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
 wrote:
> From: "Pranay Kr. Srivastava" 
>
> This patch
> 1) uses spin_lock instead of irq version.
>
> 2) removes the goto statement in case a socket
>is already assigned with simple if-else statement.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 15 +--
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 56f7f5d..766c401 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -643,17 +643,12 @@ static int nbd_set_socket(struct nbd_device *nbd, 
> struct socket *sock)
>  {
> int ret = 0;
>
> -   spin_lock_irq(>sock_lock);
> -
> -   if (nbd->sock) {
> +   spin_lock(>sock_lock);
> +   if (nbd->sock)
> ret = -EBUSY;
> -   goto out;
> -   }
> -
> -   nbd->sock = sock;
> -
> -out:
> -   spin_unlock_irq(>sock_lock);
> +   else
> +   nbd->sock = sock;
> +   spin_unlock(>sock_lock);
>
> return ret;
>  }
> --
> 1.9.1
>

Hi Markus,

Did you get a chance to review V4 of this series.

-- 
---P.K.S

Re: [PATCH 1/1]ext4: Fix WARN_ON_ONCE when marking buffer dirty

[Pranay Srivastava]

On Mon, Jul 4, 2016 at 7:59 PM, Theodore Ts'o  wrote:
> On Thu, Jun 30, 2016 at 02:12:30PM +0300, Pranay Kr. Srivastava wrote:
>> Signed-off-by: Pranay Kr. Srivastava 
>
> The description for why the change is being made should go in the
> commit.  (No need to put the description in a separate cover letter.)
> I ended up rewriting the commit description as follows, to make it
> much more understandable:
>
> ext4: Fix WARN_ON_ONCE in ext4_commit_super()
>
> If there are racing calls to ext4_commit_super() it's possible for
> another writeback of the superblock to result in the buffer being
> marked with an error after we check if the buffer is marked as
> having a write error and the buffer up-to-date flag is set again.
> If that happens mark_buffer_dirty() can end up throwing a
> WARN_ON_ONCE.
>
> Fix this by moving this check to write before we call
> write_buffer_dirty(), and keeping the buffer locked during this
> whole sequence.
>
> Signed-off-by: Pranay Kr. Srivastava 
> Signed-off-by: Theodore Ts'o 
>
> Note that the one-line summary needs to carry as much information as
> possible so someone who is scanning the commits using git log
> --oneline has a chance of understanding it.  This means the high-level
> *why* of the commit, not a summary of what the changes in the C code.
> Also note the increased context of when the misbehaviour could occur
> in the commit description, which was missing in the cover letter.
>
> When I'm processing patches, if I'm in a hurry, patches that require
> extra work or which aren't Obviously Right, sometimes get deferred by
> a few days.  This patch fell in that category.
>
> Adding to the commit descrtipion additional context and/or
> instructions for how to reproduce the problem you are trying to
> remediate will often make life much easier for me, and accelerate how
> quickly I'll get to your patch.
>
> Cheers,
>
> - Ted

Thank you Theodore Sir. Points duly noted, I'll take care from now on
while sending
patches.


-- 
---P.K.S

Re: [PATCH 1/1]ext4: Fix WARN_ON_ONCE when marking buffer dirty

[Pranay Srivastava]

On Mon, Jul 4, 2016 at 7:59 PM, Theodore Ts'o  wrote:
> On Thu, Jun 30, 2016 at 02:12:30PM +0300, Pranay Kr. Srivastava wrote:
>> Signed-off-by: Pranay Kr. Srivastava 
>
> The description for why the change is being made should go in the
> commit.  (No need to put the description in a separate cover letter.)
> I ended up rewriting the commit description as follows, to make it
> much more understandable:
>
> ext4: Fix WARN_ON_ONCE in ext4_commit_super()
>
> If there are racing calls to ext4_commit_super() it's possible for
> another writeback of the superblock to result in the buffer being
> marked with an error after we check if the buffer is marked as
> having a write error and the buffer up-to-date flag is set again.
> If that happens mark_buffer_dirty() can end up throwing a
> WARN_ON_ONCE.
>
> Fix this by moving this check to write before we call
> write_buffer_dirty(), and keeping the buffer locked during this
> whole sequence.
>
> Signed-off-by: Pranay Kr. Srivastava 
> Signed-off-by: Theodore Ts'o 
>
> Note that the one-line summary needs to carry as much information as
> possible so someone who is scanning the commits using git log
> --oneline has a chance of understanding it.  This means the high-level
> *why* of the commit, not a summary of what the changes in the C code.
> Also note the increased context of when the misbehaviour could occur
> in the commit description, which was missing in the cover letter.
>
> When I'm processing patches, if I'm in a hurry, patches that require
> extra work or which aren't Obviously Right, sometimes get deferred by
> a few days.  This patch fell in that category.
>
> Adding to the commit descrtipion additional context and/or
> instructions for how to reproduce the problem you are trying to
> remediate will often make life much easier for me, and accelerate how
> quickly I'll get to your patch.
>
> Cheers,
>
> - Ted

Thank you Theodore Sir. Points duly noted, I'll take care from now on
while sending
patches.


-- 
---P.K.S

Re: [PATCH 1/1]ext4: Fix WARN_ON_ONCE when marking buffer dirty

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:42 PM, Pranay Kr. Srivastava
 wrote:
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  fs/ext4/super.c | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3822a5a..8f10715 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4327,20 +4327,6 @@ static int ext4_commit_super(struct super_block *sb, 
> int sync)
>
> if (!sbh || block_device_ejected(sb))
> return error;
> -   if (buffer_write_io_error(sbh)) {
> -   /*
> -* Oh, dear.  A previous attempt to write the
> -* superblock failed.  This could happen because the
> -* USB device was yanked out.  Or it could happen to
> -* be a transient write error and maybe the block will
> -* be remapped.  Nothing we can do but to retry the
> -* write and hope for the best.
> -*/
> -   ext4_msg(sb, KERN_ERR, "previous I/O error to "
> -  "superblock detected");
> -   clear_buffer_write_io_error(sbh);
> -   set_buffer_uptodate(sbh);
> -   }
> /*
>  * If the file system is mounted read-only, don't update the
>  * superblock write time.  This avoids updating the superblock
> @@ -4371,7 +4357,23 @@ static int ext4_commit_super(struct super_block *sb, 
> int sync)
> _SB(sb)->s_freeinodes_counter));
> BUFFER_TRACE(sbh, "marking dirty");
> ext4_superblock_csum_set(sb);
> +   lock_buffer(sbh);
> +   if (buffer_write_io_error(sbh)) {
> +   /*
> +* Oh, dear.  A previous attempt to write the
> +* superblock failed.  This could happen because the
> +* USB device was yanked out.  Or it could happen to
> +* be a transient write error and maybe the block will
> +* be remapped.  Nothing we can do but to retry the
> +* write and hope for the best.
> +*/
> +   ext4_msg(sb, KERN_ERR, "previous I/O error to "
> +  "superblock detected");
> +   clear_buffer_write_io_error(sbh);
> +   set_buffer_uptodate(sbh);
> +   }
> mark_buffer_dirty(sbh);
> +   unlock_buffer(sbh);
> if (sync) {
> error = __sync_dirty_buffer(sbh,
> test_opt(sb, BARRIER) ? WRITE_FUA : WRITE_SYNC);
> --
> 1.9.1
>

Can this be reviewed as well please.


-- 
---P.K.S

Re: [PATCH 1/1]ext4: Fix WARN_ON_ONCE when marking buffer dirty

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:42 PM, Pranay Kr. Srivastava
 wrote:
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  fs/ext4/super.c | 30 --
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3822a5a..8f10715 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4327,20 +4327,6 @@ static int ext4_commit_super(struct super_block *sb, 
> int sync)
>
> if (!sbh || block_device_ejected(sb))
> return error;
> -   if (buffer_write_io_error(sbh)) {
> -   /*
> -* Oh, dear.  A previous attempt to write the
> -* superblock failed.  This could happen because the
> -* USB device was yanked out.  Or it could happen to
> -* be a transient write error and maybe the block will
> -* be remapped.  Nothing we can do but to retry the
> -* write and hope for the best.
> -*/
> -   ext4_msg(sb, KERN_ERR, "previous I/O error to "
> -  "superblock detected");
> -   clear_buffer_write_io_error(sbh);
> -   set_buffer_uptodate(sbh);
> -   }
> /*
>  * If the file system is mounted read-only, don't update the
>  * superblock write time.  This avoids updating the superblock
> @@ -4371,7 +4357,23 @@ static int ext4_commit_super(struct super_block *sb, 
> int sync)
> _SB(sb)->s_freeinodes_counter));
> BUFFER_TRACE(sbh, "marking dirty");
> ext4_superblock_csum_set(sb);
> +   lock_buffer(sbh);
> +   if (buffer_write_io_error(sbh)) {
> +   /*
> +* Oh, dear.  A previous attempt to write the
> +* superblock failed.  This could happen because the
> +* USB device was yanked out.  Or it could happen to
> +* be a transient write error and maybe the block will
> +* be remapped.  Nothing we can do but to retry the
> +* write and hope for the best.
> +*/
> +   ext4_msg(sb, KERN_ERR, "previous I/O error to "
> +  "superblock detected");
> +   clear_buffer_write_io_error(sbh);
> +   set_buffer_uptodate(sbh);
> +   }
> mark_buffer_dirty(sbh);
> +   unlock_buffer(sbh);
> if (sync) {
> error = __sync_dirty_buffer(sbh,
> test_opt(sb, BARRIER) ? WRITE_FUA : WRITE_SYNC);
> --
> 1.9.1
>

Can this be reviewed as well please.


-- 
---P.K.S

Re: [PATCH v4 2/5]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
 wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
> socket. This solves the issue when kernel_sendmsg is currently
> blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 57 
> +
>  1 file changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 766c401..e362d44 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /* This is specifically for calling sock_shutdown, for now. */
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +98,8 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +177,36 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
>
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   schedule_work(>ws_shutdown);
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -588,7 +590,11 @@ static int nbd_thread_send(void *data)
> spin_unlock_irq(>queue_lock);
>
> /* handle request */
> -   nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -663,6 +669,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -797,11 +804,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> error = nbd_thread_recv(nbd, bdev);
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
> +   sock_shutdown(nbd);
>
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
> @@ -857,6 +864,14 @@ static const struct block_device_operations nbd_fops = {
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> +{
> +   struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
> +   ws_shutdown);
> +
> +   sock_shutdown(nbd_dev);
> +}
> +
>  #if 

Re: [PATCH v4 2/5]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Thu, Jun 30, 2016 at 4:32 PM, Pranay Kr. Srivastava
 wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
> socket. This solves the issue when kernel_sendmsg is currently
> blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 57 
> +
>  1 file changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 766c401..e362d44 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /* This is specifically for calling sock_shutdown, for now. */
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +98,8 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +177,36 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
>
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   schedule_work(>ws_shutdown);
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -588,7 +590,11 @@ static int nbd_thread_send(void *data)
> spin_unlock_irq(>queue_lock);
>
> /* handle request */
> -   nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -663,6 +669,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -797,11 +804,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> error = nbd_thread_recv(nbd, bdev);
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
> +   sock_shutdown(nbd);
>
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
> @@ -857,6 +864,14 @@ static const struct block_device_operations nbd_fops = {
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> +{
> +   struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device,
> +   ws_shutdown);
> +
> +   sock_shutdown(nbd_dev);
> +}
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>
>  static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
> --
> 1.9.1
>

Re: [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi

On Wed, Jun 29, 2016 at 12:36 PM, Markus Pargmann  wrote:
> Hi,
>
> On Friday 24 June 2016 13:09:36 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.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>> when all tasks having this bdev open closes this bdev.
>
> Sorry, but this patch is unreadable. You are changing so many unrelated
> whitespaces, lines, comments (that you introduced yourself in a previous
> patch) and unrelated code. Please keep only the things that are

Yes you are right.

> necessary for this single patch. Everything else can go into different
> patches. Also it would be good to run checkpatch sometimes.
>

> Also using your own atomic implementation instead of kref would be good
> perhaps. Although I thought kref would work at the beginning but it
> seems not to.

I was able to make it work and it turned out to be simpler.
Do you think you can go over this patch again just for the kref part?
Should I resend this patch again?

>
> Best Regards,
>
> Markus
>
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 124 
>> 
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 9223b09..0587bbd 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -70,10 +70,13 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>   struct dentry *dbg_dir;
>>  #endif
>> +
>>   /*
>> - *This is specifically for calling sock_shutdown, for now.
>> - */
>> +  *This is specifically for calling sock_shutdown, for now.
>> +  */
>>   struct work_struct ws_shutdown;
>> + struct kref users;
>> + struct completion user_completion;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>>   * Shutdown function for nbd_dev work struct.
>>   */
>>  static void nbd_ws_func_shutdown(struct work_struct *);
>> +static void nbd_kref_release(struct kref *);
>> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>>
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>>
>>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>>  {
>> - bdev->bd_inode->i_size = 0;
>> + i_size_write(bdev->bd_inode, 0);
>>   set_capacity(nbd->disk, 0);
>>   kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>
>> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, 
>> struct block_device *bdev)
>>   if (!nbd_is_connected(nbd))
>>   return;
>>
>> - bdev->bd_inode->i_size = nbd->bytesize;
>> + i_size_write(bdev->bd_inode, nbd->bytesize);
>>   set_capacity(nbd->disk, nbd->bytesize >> 9);
>>   kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>  }
>> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct 
>> block_device *bdev,
>>   int blocksize, int nr_blocks)
>>  {
>>   int ret;
>> -
>>   ret = set_blocksize(bdev, blocksize);
>>   if (ret)
>>   return ret;
>> -
>>   nbd->blksize = blocksize;
>>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>>
>> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>>
>> + if (nbd->timedout)
>> + return;
>> +
>>   if (list_empty(>queue_head))
>>   return;
>> +
>>   nbd->timedout = true;
>> - schedule_work(>ws_shutdown);
>> +
>>   /*
>>* Make sure sender thread sees nbd->timedout.
>>*/
>>   smp_wmb();
>> + schedule_work(>ws_shutdown);
>>   wake_up(>waiting_wq);
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, 
>> struct block_device *bdev)
>>   nbd_end_request(nbd, req);
>>   }
>>
>> - nbd_size_clear(nbd, bdev);
>> -
>>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>>
>>   nbd->task_recv = NULL;
>> @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data)
>>   while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
>>   /* wait for something to do */
>>   

Re: [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi

On Wed, Jun 29, 2016 at 12:36 PM, Markus Pargmann  wrote:
> Hi,
>
> On Friday 24 June 2016 13:09:36 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.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>> when all tasks having this bdev open closes this bdev.
>
> Sorry, but this patch is unreadable. You are changing so many unrelated
> whitespaces, lines, comments (that you introduced yourself in a previous
> patch) and unrelated code. Please keep only the things that are

Yes you are right.

> necessary for this single patch. Everything else can go into different
> patches. Also it would be good to run checkpatch sometimes.
>

> Also using your own atomic implementation instead of kref would be good
> perhaps. Although I thought kref would work at the beginning but it
> seems not to.

I was able to make it work and it turned out to be simpler.
Do you think you can go over this patch again just for the kref part?
Should I resend this patch again?

>
> Best Regards,
>
> Markus
>
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 124 
>> 
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 9223b09..0587bbd 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -70,10 +70,13 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>   struct dentry *dbg_dir;
>>  #endif
>> +
>>   /*
>> - *This is specifically for calling sock_shutdown, for now.
>> - */
>> +  *This is specifically for calling sock_shutdown, for now.
>> +  */
>>   struct work_struct ws_shutdown;
>> + struct kref users;
>> + struct completion user_completion;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>>   * Shutdown function for nbd_dev work struct.
>>   */
>>  static void nbd_ws_func_shutdown(struct work_struct *);
>> +static void nbd_kref_release(struct kref *);
>> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>>
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>>
>>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>>  {
>> - bdev->bd_inode->i_size = 0;
>> + i_size_write(bdev->bd_inode, 0);
>>   set_capacity(nbd->disk, 0);
>>   kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>
>> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, 
>> struct block_device *bdev)
>>   if (!nbd_is_connected(nbd))
>>   return;
>>
>> - bdev->bd_inode->i_size = nbd->bytesize;
>> + i_size_write(bdev->bd_inode, nbd->bytesize);
>>   set_capacity(nbd->disk, nbd->bytesize >> 9);
>>   kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>>  }
>> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct 
>> block_device *bdev,
>>   int blocksize, int nr_blocks)
>>  {
>>   int ret;
>> -
>>   ret = set_blocksize(bdev, blocksize);
>>   if (ret)
>>   return ret;
>> -
>>   nbd->blksize = blocksize;
>>   nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>>
>> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>>
>> + if (nbd->timedout)
>> + return;
>> +
>>   if (list_empty(>queue_head))
>>   return;
>> +
>>   nbd->timedout = true;
>> - schedule_work(>ws_shutdown);
>> +
>>   /*
>>* Make sure sender thread sees nbd->timedout.
>>*/
>>   smp_wmb();
>> + schedule_work(>ws_shutdown);
>>   wake_up(>waiting_wq);
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, 
>> struct block_device *bdev)
>>   nbd_end_request(nbd, req);
>>   }
>>
>> - nbd_size_clear(nbd, bdev);
>> -
>>   device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>>
>>   nbd->task_recv = NULL;
>> @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data)
>>   while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
>>   /* wait for something to do */
>>   wait_event_interruptible(nbd->waiting_wq,
>> - 

Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

Hi Markus,

On Fri, Jun 24, 2016 at 3:39 PM, Pranay Kr. Srivastava
 wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 69 
> ++---
>  1 file changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 56f7f5d..586d946 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +   *This is specifically for calling sock_shutdown, for now.
> +   */
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -574,8 +580,8 @@ static int nbd_thread_send(void *data)
> while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
> -kthread_should_stop() ||
> -!list_empty(>waiting_queue));
> +   kthread_should_stop() ||
> +   !list_empty(>waiting_queue));
>
> /* extract request */
> if (list_empty(>waiting_queue))
> @@ -583,12 +589,16 @@ static int nbd_thread_send(void *data)
>
> spin_lock_irq(>queue_lock);
> req = list_entry(nbd->waiting_queue.next, struct request,
> -queuelist);
> +   queuelist);
> list_del_init(>queuelist);
> spin_unlock_irq(>queue_lock);
>
> -   /* handle request */
> nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -668,6 +678,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> 

Re: [PATCH v3 1/3]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

Hi Markus,

On Fri, Jun 24, 2016 at 3:39 PM, Pranay Kr. Srivastava
 wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 69 
> ++---
>  1 file changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 56f7f5d..586d946 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +   *This is specifically for calling sock_shutdown, for now.
> +   */
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -574,8 +580,8 @@ static int nbd_thread_send(void *data)
> while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
> -kthread_should_stop() ||
> -!list_empty(>waiting_queue));
> +   kthread_should_stop() ||
> +   !list_empty(>waiting_queue));
>
> /* extract request */
> if (list_empty(>waiting_queue))
> @@ -583,12 +589,16 @@ static int nbd_thread_send(void *data)
>
> spin_lock_irq(>queue_lock);
> req = list_entry(nbd->waiting_queue.next, struct request,
> -queuelist);
> +   queuelist);
> list_del_init(>queuelist);
> spin_unlock_irq(>queue_lock);
>
> -   /* handle request */
> nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -668,6 +678,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -802,11 +813,11 @@ 

Re: [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi

On Fri, Jun 24, 2016 at 3:39 PM, 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.
>
> Each open of a nbd device is refcounted, while
> the userland program [nbd-client] doing the
> NBD_DO_IT ioctl would now wait for any other users
> of this device before invalidating the nbd device.
>
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The resetting happens
> when all tasks having this bdev open closes this bdev.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 124 
> 
>  1 file changed, 96 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9223b09..0587bbd 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -70,10 +70,13 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +
> /*
> -   *This is specifically for calling sock_shutdown, for now.
> -   */
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> struct work_struct ws_shutdown;
> +   struct kref users;
> +   struct completion user_completion;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>   * Shutdown function for nbd_dev work struct.
>   */
>  static void nbd_ws_func_shutdown(struct work_struct *);
> +static void nbd_kref_release(struct kref *);
> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>
>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>  {
> -   bdev->bd_inode->i_size = 0;
> +   i_size_write(bdev->bd_inode, 0);
> set_capacity(nbd->disk, 0);
> kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>
> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, 
> struct block_device *bdev)
> if (!nbd_is_connected(nbd))
> return;
>
> -   bdev->bd_inode->i_size = nbd->bytesize;
> +   i_size_write(bdev->bd_inode, nbd->bytesize);
> set_capacity(nbd->disk, nbd->bytesize >> 9);
> kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>  }
> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct 
> block_device *bdev,
> int blocksize, int nr_blocks)
>  {
> int ret;
> -
> ret = set_blocksize(bdev, blocksize);
> if (ret)
> return ret;
> -
> nbd->blksize = blocksize;
> nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>
> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
>
> +   if (nbd->timedout)
> +   return;
> +
> if (list_empty(>queue_head))
> return;
> +
> nbd->timedout = true;
> -   schedule_work(>ws_shutdown);
> +
> /*
>  * Make sure sender thread sees nbd->timedout.
>  */
> smp_wmb();
> +   schedule_work(>ws_shutdown);
> wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct 
> block_device *bdev)
> nbd_end_request(nbd, req);
> }
>
> -   nbd_size_clear(nbd, bdev);
> -
> device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>
> nbd->task_recv = NULL;
> @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data)
> while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
> -   kthread_should_stop() ||
> -   !list_empty(>waiting_queue));
> +kthread_should_stop() ||
> +!list_empty(>waiting_queue));
>
> /* extract request */
> if (list_empty(>waiting_queue))
> @@ -589,11 +595,11 @@ static int nbd_thread_send(void *data)
>
> spin_lock_irq(>queue_lock);
> req = list_entry(nbd->waiting_queue.next, struct request,
> -   queuelist);
> +queuelist);
> list_del_init(>queuelist);
> spin_unlock_irq(>queue_lock);
>
> -   nbd_handle_req(nbd, req);
> +   /* handle 

Re: [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi

On Fri, Jun 24, 2016 at 3:39 PM, 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.
>
> Each open of a nbd device is refcounted, while
> the userland program [nbd-client] doing the
> NBD_DO_IT ioctl would now wait for any other users
> of this device before invalidating the nbd device.
>
> A timedout or a disconnected device, if in use, can't
> be used until it has been resetted. The resetting happens
> when all tasks having this bdev open closes this bdev.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 124 
> 
>  1 file changed, 96 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9223b09..0587bbd 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -70,10 +70,13 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +
> /*
> -   *This is specifically for calling sock_shutdown, for now.
> -   */
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> struct work_struct ws_shutdown;
> +   struct kref users;
> +   struct completion user_completion;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock);
>   * Shutdown function for nbd_dev work struct.
>   */
>  static void nbd_ws_func_shutdown(struct work_struct *);
> +static void nbd_kref_release(struct kref *);
> +static int nbd_size_clear(struct nbd_device *, struct block_device *);
>
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd)
>
>  static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
>  {
> -   bdev->bd_inode->i_size = 0;
> +   i_size_write(bdev->bd_inode, 0);
> set_capacity(nbd->disk, 0);
> kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>
> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, 
> struct block_device *bdev)
> if (!nbd_is_connected(nbd))
> return;
>
> -   bdev->bd_inode->i_size = nbd->bytesize;
> +   i_size_write(bdev->bd_inode, nbd->bytesize);
> set_capacity(nbd->disk, nbd->bytesize >> 9);
> kobject_uevent(_to_dev(nbd)->kobj, KOBJ_CHANGE);
>  }
> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct 
> block_device *bdev,
> int blocksize, int nr_blocks)
>  {
> int ret;
> -
> ret = set_blocksize(bdev, blocksize);
> if (ret)
> return ret;
> -
> nbd->blksize = blocksize;
> nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks;
>
> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
>
> +   if (nbd->timedout)
> +   return;
> +
> if (list_empty(>queue_head))
> return;
> +
> nbd->timedout = true;
> -   schedule_work(>ws_shutdown);
> +
> /*
>  * Make sure sender thread sees nbd->timedout.
>  */
> smp_wmb();
> +   schedule_work(>ws_shutdown);
> wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct 
> block_device *bdev)
> nbd_end_request(nbd, req);
> }
>
> -   nbd_size_clear(nbd, bdev);
> -
> device_remove_file(disk_to_dev(nbd->disk), _attr_pid);
>
> nbd->task_recv = NULL;
> @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data)
> while (!kthread_should_stop() || !list_empty(>waiting_queue)) {
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
> -   kthread_should_stop() ||
> -   !list_empty(>waiting_queue));
> +kthread_should_stop() ||
> +!list_empty(>waiting_queue));
>
> /* extract request */
> if (list_empty(>waiting_queue))
> @@ -589,11 +595,11 @@ static int nbd_thread_send(void *data)
>
> spin_lock_irq(>queue_lock);
> req = list_entry(nbd->waiting_queue.next, struct request,
> -   queuelist);
> +queuelist);
> list_del_init(>queuelist);
> spin_unlock_irq(>queue_lock);
>
> -   nbd_handle_req(nbd, req);
> +   /* handle request */
> if 

Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi Eric,

On Fri, Jun 24, 2016 at 7:12 PM, Eric Blake  wrote:
> On 06/24/2016 04:09 AM, Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>
> s/abruplty/abruptly/
>
>> wait for it's users to finish.
>
> s/it's/its/
>
>>
>> This is more required when filesystem(s) like
>> ext2 or ext3 don't expect their buffer heads to
>> disappear while the filesystem is mounted.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>
> s/resetted/reset/
>

Thanks for going through the patch. Can I get some review on the
code as well so I can fix and resend that too, along with fixes of grammatical
errors.

>> when all tasks having this bdev open closes this bdev.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 124 
>> 
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
---P.K.S

Re: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users

[Pranay Srivastava]

Hi Eric,

On Fri, Jun 24, 2016 at 7:12 PM, Eric Blake  wrote:
> On 06/24/2016 04:09 AM, Pranay Kr. Srivastava wrote:
>> When a timeout occurs or a recv fails, then
>> instead of abruplty killing nbd block device
>
> s/abruplty/abruptly/
>
>> wait for it's users to finish.
>
> s/it's/its/
>
>>
>> This is more required when filesystem(s) like
>> ext2 or ext3 don't expect their buffer heads to
>> disappear while the filesystem is mounted.
>>
>> Each open of a nbd device is refcounted, while
>> the userland program [nbd-client] doing the
>> NBD_DO_IT ioctl would now wait for any other users
>> of this device before invalidating the nbd device.
>>
>> A timedout or a disconnected device, if in use, can't
>> be used until it has been resetted. The resetting happens
>
> s/resetted/reset/
>

Thanks for going through the patch. Can I get some review on the
code as well so I can fix and resend that too, along with fixes of grammatical
errors.

>> when all tasks having this bdev open closes this bdev.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 124 
>> 
>>  1 file changed, 96 insertions(+), 28 deletions(-)
>>
>
>
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
---P.K.S

Re: [PATCH 1/2] nbd: make nbd device wait for its users

[Pranay Srivastava]

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> From: "Pranay Kr. Srivastava" 
>
> 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.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava 
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann 
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I 
> wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 
> ++---
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   atomic_t users; /* Users that opened the block device */

We don't need to put bdev in nbd struct. We can get it via gendisk and
bdget/bdput.
> +   struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct 
> socket *sock)
> return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +   set_device_ro(bdev, false);
> +   bdev->bd_inode->i_size = 0;

i_size_write should be better I guess.

> +   if (max_part > 0) {
> +   blkdev_reread_part(bdev);
> +   bdev->bd_invalidated = 1;
> +   }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   if (nbd->bdev) {
> +   kill_bdev(nbd->bdev);
> +   nbd_bdev_reset(nbd->bdev);
> +   }
> +
> nbd->disconnect = false;
> nbd->timedout = false;
> nbd->blksize = 1024;

I actually forgot to ask about this blksize. We do set_blocksize call
for bdev, but
shouldn't we instead do blkdev_logicial_blocksize?

Mount would set the blocksize depending on the filesystem's block size and the
block device logical block size supported. Should we just get rid of this?

> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
> del_timer_sync(>timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -   set_device_ro(bdev, false);
> -   bdev->bd_inode->i_size = 0;
> -   if (max_part > 0) {
> -   blkdev_reread_part(bdev);
> -   bdev->bd_invalidated = 1;
> -   }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device 
> *bdev)
>  {
> if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,

How about disallowing any ioctl until the nbd_device has been reset?
Perhaps, throw error
in open instead of checking here?

> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
> -
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>

Doesn't this change the semantics for user space? NBD_DO_IT is
supposed to be over either on error or a
user disconnect not before that right[?].

> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -853,10 +855,35 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   if (!nbd->bdev)
> +   nbd->bdev = bdev;
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +
> +   if (atomic_dec_and_test(>users)) {
> +   

Re: [PATCH 1/2] nbd: make nbd device wait for its users

[Pranay Srivastava]

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> From: "Pranay Kr. Srivastava" 
>
> 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.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava 
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann 
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I 
> wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 
> ++---
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   atomic_t users; /* Users that opened the block device */

We don't need to put bdev in nbd struct. We can get it via gendisk and
bdget/bdput.
> +   struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct 
> socket *sock)
> return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +   set_device_ro(bdev, false);
> +   bdev->bd_inode->i_size = 0;

i_size_write should be better I guess.

> +   if (max_part > 0) {
> +   blkdev_reread_part(bdev);
> +   bdev->bd_invalidated = 1;
> +   }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   if (nbd->bdev) {
> +   kill_bdev(nbd->bdev);
> +   nbd_bdev_reset(nbd->bdev);
> +   }
> +
> nbd->disconnect = false;
> nbd->timedout = false;
> nbd->blksize = 1024;

I actually forgot to ask about this blksize. We do set_blocksize call
for bdev, but
shouldn't we instead do blkdev_logicial_blocksize?

Mount would set the blocksize depending on the filesystem's block size and the
block device logical block size supported. Should we just get rid of this?

> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
> del_timer_sync(>timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -   set_device_ro(bdev, false);
> -   bdev->bd_inode->i_size = 0;
> -   if (max_part > 0) {
> -   blkdev_reread_part(bdev);
> -   bdev->bd_invalidated = 1;
> -   }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device 
> *bdev)
>  {
> if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,

How about disallowing any ioctl until the nbd_device has been reset?
Perhaps, throw error
in open instead of checking here?

> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
> -
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>

Doesn't this change the semantics for user space? NBD_DO_IT is
supposed to be over either on error or a
user disconnect not before that right[?].

> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -853,10 +855,35 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   if (!nbd->bdev)
> +   nbd->bdev = bdev;
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +
> +   if (atomic_dec_and_test(>users)) {
> +   mutex_lock(>tx_lock);
> +   nbd_reset(nbd);
> +   

Re: [PATCH 1/2] nbd: make nbd device wait for its users

[Pranay Srivastava]

Hey Markus,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> From: "Pranay Kr. Srivastava" 
>
> 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.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava 
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann 
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I 
> wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 
> ++---
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   atomic_t users; /* Users that opened the block device */
> +   struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct 
> socket *sock)
> return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +   set_device_ro(bdev, false);
> +   bdev->bd_inode->i_size = 0;
> +   if (max_part > 0) {
> +   blkdev_reread_part(bdev);
> +   bdev->bd_invalidated = 1;
> +   }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   if (nbd->bdev) {
> +   kill_bdev(nbd->bdev);
> +   nbd_bdev_reset(nbd->bdev);
> +   }
> +
> nbd->disconnect = false;
> nbd->timedout = false;
> nbd->blksize = 1024;
> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
> del_timer_sync(>timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -   set_device_ro(bdev, false);
> -   bdev->bd_inode->i_size = 0;
> -   if (max_part > 0) {
> -   blkdev_reread_part(bdev);
> -   bdev->bd_invalidated = 1;
> -   }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device 
> *bdev)
>  {
> if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
> -
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -853,10 +855,35 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   if (!nbd->bdev)
> +   nbd->bdev = bdev;
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +
> +   if (atomic_dec_and_test(>users)) {
> +   mutex_lock(>tx_lock);
> +   nbd_reset(nbd);
> +   mutex_unlock(>tx_lock);
> +   }
> +}
> +
>  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)
> @@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
> disk->private_data = _dev[i];
> sprintf(disk->disk_name, "nbd%d", i);
> nbd_reset(_dev[i]);
> +   atomic_set(_dev[i].users, 0);
> 

Re: [PATCH 1/2] nbd: make nbd device wait for its users

[Pranay Srivastava]

Hey Markus,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann  wrote:
> From: "Pranay Kr. Srivastava" 
>
> 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.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava 
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann 
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I 
> wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 
> ++---
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   atomic_t users; /* Users that opened the block device */
> +   struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct 
> socket *sock)
> return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +   set_device_ro(bdev, false);
> +   bdev->bd_inode->i_size = 0;
> +   if (max_part > 0) {
> +   blkdev_reread_part(bdev);
> +   bdev->bd_invalidated = 1;
> +   }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +   sock_shutdown(nbd);
> +   nbd_clear_que(nbd);
> +   if (nbd->bdev) {
> +   kill_bdev(nbd->bdev);
> +   nbd_bdev_reset(nbd->bdev);
> +   }
> +
> nbd->disconnect = false;
> nbd->timedout = false;
> nbd->blksize = 1024;
> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
> del_timer_sync(>timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -   set_device_ro(bdev, false);
> -   bdev->bd_inode->i_size = 0;
> -   if (max_part > 0) {
> -   blkdev_reread_part(bdev);
> -   bdev->bd_invalidated = 1;
> -   }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device 
> *bdev)
>  {
> if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> mutex_lock(>tx_lock);
> nbd->task_recv = NULL;
>
> -   sock_shutdown(nbd);
> -   nbd_clear_que(nbd);
> -   kill_bdev(bdev);
> -   nbd_bdev_reset(bdev);
> -
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> -   nbd_reset(nbd);
> -
> return error;
> }
>
> @@ -853,10 +855,35 @@ 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 = bdev->bd_disk->private_data;
> +
> +   atomic_inc(>users);
> +
> +   if (!nbd->bdev)
> +   nbd->bdev = bdev;
> +
> +   return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +   struct nbd_device *nbd = disk->private_data;
> +
> +   if (atomic_dec_and_test(>users)) {
> +   mutex_lock(>tx_lock);
> +   nbd_reset(nbd);
> +   mutex_unlock(>tx_lock);
> +   }
> +}
> +
>  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)
> @@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
> disk->private_data = _dev[i];
> sprintf(disk->disk_name, "nbd%d", i);
> nbd_reset(_dev[i]);
> +   atomic_set(_dev[i].users, 0);
> add_disk(disk);
> }
>
> --
> 2.1.4
>

I'll be sending out rebased patch today, 

Re: [Nbd] [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

On Wed, Jun 15, 2016 at 12:30 PM, Wouter Verhelst  wrote:
> On Wed, Jun 15, 2016 at 08:30:45AM +0200, Markus Pargmann wrote:
>> Thanks for the explanations. I think my understanding was off by one ;)..
>> I didn't realize that the DO_IT thread from the userspace has the block
>> device open as well.
>
> Obviously, otherwise it couldn't do an ioctl() to it :-)
>
>> I thought a bit about this, does it make sense to delay the essential
>> cleanup steps until really all open file handles were closed? So that
>> even if the DO_IT thread exits, the block device is still there. Only if
>> the file is closed everything is cleaned up. Maybe this makes the code
>> simpler and we can directly use krefs without any strange constructs.
>> What do you think?


>>
>> This would also allow the client to setup a new socket as long as it
>> does not close the nbd file handle.
>
> That sounds like the behaviour that I described earlier about possible
> retries for userspace...
>
>> Could this behavior be potentially problematic for any client
>> implementation?
>
> I don't think it could, but I'm not sure I understand all the details.
> What would happen if:
>
> - nbd is connected from pid X, pid Y does NBD_DISCONNECT, pid X hangs
>   and doesn't exit?

In that case pid X would have an error on recv if I'am correct. Then if no other
users[like mounts or other user space applications] are present for this device
then pidx would exit.

> - nbd is connected from pid X, server disconnects while pid Y is trying
>   to access the device, pid X tries to reconnect but it takes a while?

Not sure what issue you see in the above case but if I understand correctly,

This should again fall in error case [Correct?] as a result of a timeout may be
if any requests were in progress. I don't think reconnect will be
without any error
thrown up user space, not so sure if both sides [server and client]
were sitting idle.
OR
if no timeout was there then things should go OK after a successful reconnect.

Multiple processes can still access the device.

>
>> Does it solve our other issue with setting up a new sockets for an
>> existing nbd blockdevice?
>
> It could, depending.

This should be OK.

>
> --
> < ron> I mean, the main *practical* problem with C++, is there's like a dozen
>people in the world who think they really understand all of its rules,
>and pretty much all of them are just lying to themselves too.
>  -- #debian-devel, OFTC, 2016-02-12



-- 
---P.K.S

Re: [Nbd] [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

On Wed, Jun 15, 2016 at 12:30 PM, Wouter Verhelst  wrote:
> On Wed, Jun 15, 2016 at 08:30:45AM +0200, Markus Pargmann wrote:
>> Thanks for the explanations. I think my understanding was off by one ;)..
>> I didn't realize that the DO_IT thread from the userspace has the block
>> device open as well.
>
> Obviously, otherwise it couldn't do an ioctl() to it :-)
>
>> I thought a bit about this, does it make sense to delay the essential
>> cleanup steps until really all open file handles were closed? So that
>> even if the DO_IT thread exits, the block device is still there. Only if
>> the file is closed everything is cleaned up. Maybe this makes the code
>> simpler and we can directly use krefs without any strange constructs.
>> What do you think?


>>
>> This would also allow the client to setup a new socket as long as it
>> does not close the nbd file handle.
>
> That sounds like the behaviour that I described earlier about possible
> retries for userspace...
>
>> Could this behavior be potentially problematic for any client
>> implementation?
>
> I don't think it could, but I'm not sure I understand all the details.
> What would happen if:
>
> - nbd is connected from pid X, pid Y does NBD_DISCONNECT, pid X hangs
>   and doesn't exit?

In that case pid X would have an error on recv if I'am correct. Then if no other
users[like mounts or other user space applications] are present for this device
then pidx would exit.

> - nbd is connected from pid X, server disconnects while pid Y is trying
>   to access the device, pid X tries to reconnect but it takes a while?

Not sure what issue you see in the above case but if I understand correctly,

This should again fall in error case [Correct?] as a result of a timeout may be
if any requests were in progress. I don't think reconnect will be
without any error
thrown up user space, not so sure if both sides [server and client]
were sitting idle.
OR
if no timeout was there then things should go OK after a successful reconnect.

Multiple processes can still access the device.

>
>> Does it solve our other issue with setting up a new sockets for an
>> existing nbd blockdevice?
>
> It could, depending.

This should be OK.

>
> --
> < ron> I mean, the main *practical* problem with C++, is there's like a dozen
>people in the world who think they really understand all of its rules,
>and pretty much all of them are just lying to themselves too.
>  -- #debian-devel, OFTC, 2016-02-12



-- 
---P.K.S

Re: [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

Hey Markus,

On Wed, Jun 15, 2016 at 12:00 PM, Markus Pargmann <m...@pengutronix.de> wrote:
> Hi Pranay,
>
> On Tuesday 14 June 2016 15:03:40 Pranay Srivastava wrote:
>> Hi Markus,
>>
>> On Tue, Jun 14, 2016 at 2:29 PM, Markus Pargmann <m...@pengutronix.de> wrote:
>> >
>> > On Thursday 02 June 2016 13:25:00 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.
>> > >
>> > > Each open of a nbd device is refcounted, while
>> > > the userland program [nbd-client] doing the
>> > > NBD_DO_IT ioctl would now wait for any other users
>> > > of this device before invalidating the nbd device.
>> > >
>> > > Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>> > > ---
>> > >  drivers/block/nbd.c | 58 
>> > > +
>> > >  1 file changed, 58 insertions(+)
>> > >
>> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > > index d1d898d..4da40dc 100644
>> > > --- a/drivers/block/nbd.c
>> > > +++ b/drivers/block/nbd.c
>> > > @@ -70,10 +70,13 @@ struct nbd_device {
>> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> > >   struct dentry *dbg_dir;
>> > >  #endif
>> > > + atomic_t inuse;
>> > >   /*
>> > >*This is specifically for calling sock_shutdown, for now.
>> > >*/
>> > >   struct work_struct ws_shutdown;
>> > > + struct kref users;
>> > > + struct completion user_completion;
>> > >  };
>> > >
>> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> > > @@ -104,6 +107,7 @@ static DEFINE_SPINLOCK(nbd_lock);
>> > >   * Shutdown function for nbd_dev work struct.
>> > >   */
>> > >  static void nbd_ws_func_shutdown(struct work_struct *);
>> > > +static void nbd_kref_release(struct kref *);
>> > >
>> > >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> > >  {
>> > > @@ -682,6 +686,8 @@ static void nbd_reset(struct nbd_device *nbd)
>> > >   nbd->flags = 0;
>> > >   nbd->xmit_timeout = 0;
>> > >   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>> > > + init_completion(>user_completion);
>> > > + kref_init(>users);
>> > >   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>> > >   del_timer_sync(>timeout_timer);
>> > >  }
>> > > @@ -815,6 +821,14 @@ static int __nbd_ioctl(struct block_device *bdev, 
>> > > struct nbd_device *nbd,
>> > >   kthread_stop(thread);
>> > >
>> > >   sock_shutdown(nbd);
>> > > + /*
>> > > +  * kref_init initializes with ref count as 1,
>> > > +  * nbd_client, or the user-land program executing
>> > > +  * this ioctl will make the refcount to 2[at least]
>> > > +  * so subtracting 2 from refcount.
>> > > +  */
>> > > + kref_sub(>users, 2, nbd_kref_release);
>> >
>> > Why don't you use a kref_put?
>>
>> Ok, so I'll try to explain as I've understood the problem.
>>
>> When the module is loaded the kref is initialized to 1.
>>
>> Suppose now, someone has started nbd-client [nbdC-1] , then this
>> nbd-client will increase the ref count to 2. So far so good...
>>
>> Now let's say this device is being shutdown via nbd-client[nbdC-2].
>>
>> nbdC-1 will subtract the refcount by two, it has to do in NBD_DO_IT
>> since device file will not
>> be closed until after ioctl is over, and it'll wait_for_completion.
>>
>> nbdC-2 now closes it's use of device file, this makes the refcount as
>> zero and completion
>> is triggered with nbdC-1 completed.
>>
>> Now we don't want to trigger kref_put when nbdC-1 closes the device
>> file so kref_put needs
>> to be conditional in this regard so for that in_use is used.
>>
>>
>> >
>> > > 

Re: [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

Hey Markus,

On Wed, Jun 15, 2016 at 12:00 PM, Markus Pargmann  wrote:
> Hi Pranay,
>
> On Tuesday 14 June 2016 15:03:40 Pranay Srivastava wrote:
>> Hi Markus,
>>
>> On Tue, Jun 14, 2016 at 2:29 PM, Markus Pargmann  wrote:
>> >
>> > On Thursday 02 June 2016 13:25:00 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.
>> > >
>> > > Each open of a nbd device is refcounted, while
>> > > the userland program [nbd-client] doing the
>> > > NBD_DO_IT ioctl would now wait for any other users
>> > > of this device before invalidating the nbd device.
>> > >
>> > > Signed-off-by: Pranay Kr. Srivastava 
>> > > ---
>> > >  drivers/block/nbd.c | 58 
>> > > +
>> > >  1 file changed, 58 insertions(+)
>> > >
>> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > > index d1d898d..4da40dc 100644
>> > > --- a/drivers/block/nbd.c
>> > > +++ b/drivers/block/nbd.c
>> > > @@ -70,10 +70,13 @@ struct nbd_device {
>> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> > >   struct dentry *dbg_dir;
>> > >  #endif
>> > > + atomic_t inuse;
>> > >   /*
>> > >*This is specifically for calling sock_shutdown, for now.
>> > >*/
>> > >   struct work_struct ws_shutdown;
>> > > + struct kref users;
>> > > + struct completion user_completion;
>> > >  };
>> > >
>> > >  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> > > @@ -104,6 +107,7 @@ static DEFINE_SPINLOCK(nbd_lock);
>> > >   * Shutdown function for nbd_dev work struct.
>> > >   */
>> > >  static void nbd_ws_func_shutdown(struct work_struct *);
>> > > +static void nbd_kref_release(struct kref *);
>> > >
>> > >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>> > >  {
>> > > @@ -682,6 +686,8 @@ static void nbd_reset(struct nbd_device *nbd)
>> > >   nbd->flags = 0;
>> > >   nbd->xmit_timeout = 0;
>> > >   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>> > > + init_completion(>user_completion);
>> > > + kref_init(>users);
>> > >   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>> > >   del_timer_sync(>timeout_timer);
>> > >  }
>> > > @@ -815,6 +821,14 @@ static int __nbd_ioctl(struct block_device *bdev, 
>> > > struct nbd_device *nbd,
>> > >   kthread_stop(thread);
>> > >
>> > >   sock_shutdown(nbd);
>> > > + /*
>> > > +  * kref_init initializes with ref count as 1,
>> > > +  * nbd_client, or the user-land program executing
>> > > +  * this ioctl will make the refcount to 2[at least]
>> > > +  * so subtracting 2 from refcount.
>> > > +  */
>> > > + kref_sub(>users, 2, nbd_kref_release);
>> >
>> > Why don't you use a kref_put?
>>
>> Ok, so I'll try to explain as I've understood the problem.
>>
>> When the module is loaded the kref is initialized to 1.
>>
>> Suppose now, someone has started nbd-client [nbdC-1] , then this
>> nbd-client will increase the ref count to 2. So far so good...
>>
>> Now let's say this device is being shutdown via nbd-client[nbdC-2].
>>
>> nbdC-1 will subtract the refcount by two, it has to do in NBD_DO_IT
>> since device file will not
>> be closed until after ioctl is over, and it'll wait_for_completion.
>>
>> nbdC-2 now closes it's use of device file, this makes the refcount as
>> zero and completion
>> is triggered with nbdC-1 completed.
>>
>> Now we don't want to trigger kref_put when nbdC-1 closes the device
>> file so kref_put needs
>> to be conditional in this regard so for that in_use is used.
>>
>>
>> >
>> > > + wait_for_completion(>user_completion);
>> > >

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hi

On Tue, Jun 14, 2016 at 2:22 PM, Markus Pargmann  wrote:
> Hi,
>
> On Thursday 02 June 2016 13:24:57 Pranay Kr. Srivastava wrote:
>> spinlocked ranges should be small and not contain calls into huge
>> subfunctions. Fix my mistake and just get the pointer to the socket
>> instead of doing everything with spinlock held.
>>
>> Reported-by: Mikulas Patocka 
>> Signed-off-by: Markus Pargmann 
>>
>> Changelog:
>> Pranay Kr. Srivastava:
>>
>> 1) Use spin_lock instead of irq version for sock_shutdown.
>>
>> 2) Use system work queue to actually trigger the shutdown of
>>socket. This solves the issue when kernel_sendmsg is currently
>>blocked while a timeout occurs.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>
> This looks better. Some smaller things inline. Also this patch does not
> apply on my tree anymore. Can you please rebase it onto:
> 
> http://git.pengutronix.de/?p=mpa/linux-nbd.git;a=shortlog;h=refs/heads/master
>

Ok will do.

>> ---
>>  drivers/block/nbd.c | 65 
>> ++---
>>  1 file changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 31e73a7..0339d40 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -39,6 +39,7 @@
>>  #include 
>>
>>  #include 
>> +#include 
>>
>>  struct nbd_device {
>>   u32 flags;
>> @@ -69,6 +70,10 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>   struct dentry *dbg_dir;
>>  #endif
>> + /*
>> +  *This is specifically for calling sock_shutdown, for now.
>> +  */
>> + struct work_struct ws_shutdown;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -95,6 +100,11 @@ static int max_part;
>>   */
>>  static DEFINE_SPINLOCK(nbd_lock);
>>
>> +/*
>> + * Shutdown function for nbd_dev work struct.
>> + */
>> +static void nbd_ws_func_shutdown(struct work_struct *);
>> +
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>>   return disk_to_dev(nbd->disk);
>> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> struct request *req)
>>   */
>>  static void sock_shutdown(struct nbd_device *nbd)
>>  {
>> - spin_lock_irq(>sock_lock);
>> -
>> - if (!nbd->sock) {
>> - spin_unlock_irq(>sock_lock);
>> - return;
>> - }
>> + struct socket *sock;
>>
>> - dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> - sockfd_put(nbd->sock);
>> + spin_lock(>sock_lock);
>> + sock = nbd->sock;
>>   nbd->sock = NULL;
>> - spin_unlock_irq(>sock_lock);
>> + spin_unlock(>sock_lock);
>> +
>> + if (!sock)
>> + return;
>>
>>   del_timer(>timeout_timer);
>> + dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> + kernel_sock_shutdown(sock, SHUT_RDWR);
>> + sockfd_put(sock);
>>  }
>>
>>  static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> - unsigned long flags;
>>
>>   if (list_empty(>queue_head))
>>   return;
>> -
>> - spin_lock_irqsave(>sock_lock, flags);
>> -
>>   nbd->timedout = true;
>> -
>> - if (nbd->sock)
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> -
>> - spin_unlock_irqrestore(>sock_lock, flags);
>> -
>> + schedule_work(>ws_shutdown);
>> + /*
>> +  * Make sure sender thread sees nbd->timedout.
>> +  */
>> + smp_wmb();
>
> I am not sure that we need this memory barrier here. But as it is just
> the timeout path it probably won't hurt.
>
>> + wake_up(>waiting_wq);
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>>
>> @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
>>   spin_unlock_irq(>queue_lock);
>>
>>   /* handle request */
>> - nbd_handle_req(nbd, req);
>> + if (nbd->timedout) {
>> + req->errors++;
>> + nbd_end_request(nbd, req);
>> + } else
>> + nbd_handle_req(nbd, req);
>>   }
>>
>>   nbd->task_send = NULL;
>> @@ -672,6 +682,7 @@ static void nbd_reset(struct nbd_device *nbd)
>>   set_capacity(nbd->disk, 0);
>>   nbd->flags = 0;
>>   nbd->xmit_timeout = 0;
>> + INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>>   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>>   del_timer_sync(>timeout_timer);
>>  }
>> @@ -804,15 +815,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
>> struct nbd_device *nbd,
>>   nbd_dev_dbg_close(nbd);
>>   kthread_stop(thread);
>>
>> - mutex_lock(>tx_lock);
>> -
>>   sock_shutdown(nbd);
>> + mutex_lock(>tx_lock);
>>   nbd_clear_que(nbd);

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hi

On Tue, Jun 14, 2016 at 2:22 PM, Markus Pargmann  wrote:
> Hi,
>
> On Thursday 02 June 2016 13:24:57 Pranay Kr. Srivastava wrote:
>> spinlocked ranges should be small and not contain calls into huge
>> subfunctions. Fix my mistake and just get the pointer to the socket
>> instead of doing everything with spinlock held.
>>
>> Reported-by: Mikulas Patocka 
>> Signed-off-by: Markus Pargmann 
>>
>> Changelog:
>> Pranay Kr. Srivastava:
>>
>> 1) Use spin_lock instead of irq version for sock_shutdown.
>>
>> 2) Use system work queue to actually trigger the shutdown of
>>socket. This solves the issue when kernel_sendmsg is currently
>>blocked while a timeout occurs.
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>
> This looks better. Some smaller things inline. Also this patch does not
> apply on my tree anymore. Can you please rebase it onto:
> 
> http://git.pengutronix.de/?p=mpa/linux-nbd.git;a=shortlog;h=refs/heads/master
>

Ok will do.

>> ---
>>  drivers/block/nbd.c | 65 
>> ++---
>>  1 file changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 31e73a7..0339d40 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -39,6 +39,7 @@
>>  #include 
>>
>>  #include 
>> +#include 
>>
>>  struct nbd_device {
>>   u32 flags;
>> @@ -69,6 +70,10 @@ struct nbd_device {
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>>   struct dentry *dbg_dir;
>>  #endif
>> + /*
>> +  *This is specifically for calling sock_shutdown, for now.
>> +  */
>> + struct work_struct ws_shutdown;
>>  };
>>
>>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>> @@ -95,6 +100,11 @@ static int max_part;
>>   */
>>  static DEFINE_SPINLOCK(nbd_lock);
>>
>> +/*
>> + * Shutdown function for nbd_dev work struct.
>> + */
>> +static void nbd_ws_func_shutdown(struct work_struct *);
>> +
>>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>>  {
>>   return disk_to_dev(nbd->disk);
>> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> struct request *req)
>>   */
>>  static void sock_shutdown(struct nbd_device *nbd)
>>  {
>> - spin_lock_irq(>sock_lock);
>> -
>> - if (!nbd->sock) {
>> - spin_unlock_irq(>sock_lock);
>> - return;
>> - }
>> + struct socket *sock;
>>
>> - dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> - sockfd_put(nbd->sock);
>> + spin_lock(>sock_lock);
>> + sock = nbd->sock;
>>   nbd->sock = NULL;
>> - spin_unlock_irq(>sock_lock);
>> + spin_unlock(>sock_lock);
>> +
>> + if (!sock)
>> + return;
>>
>>   del_timer(>timeout_timer);
>> + dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> + kernel_sock_shutdown(sock, SHUT_RDWR);
>> + sockfd_put(sock);
>>  }
>>
>>  static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> - unsigned long flags;
>>
>>   if (list_empty(>queue_head))
>>   return;
>> -
>> - spin_lock_irqsave(>sock_lock, flags);
>> -
>>   nbd->timedout = true;
>> -
>> - if (nbd->sock)
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> -
>> - spin_unlock_irqrestore(>sock_lock, flags);
>> -
>> + schedule_work(>ws_shutdown);
>> + /*
>> +  * Make sure sender thread sees nbd->timedout.
>> +  */
>> + smp_wmb();
>
> I am not sure that we need this memory barrier here. But as it is just
> the timeout path it probably won't hurt.
>
>> + wake_up(>waiting_wq);
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>>
>> @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
>>   spin_unlock_irq(>queue_lock);
>>
>>   /* handle request */
>> - nbd_handle_req(nbd, req);
>> + if (nbd->timedout) {
>> + req->errors++;
>> + nbd_end_request(nbd, req);
>> + } else
>> + nbd_handle_req(nbd, req);
>>   }
>>
>>   nbd->task_send = NULL;
>> @@ -672,6 +682,7 @@ static void nbd_reset(struct nbd_device *nbd)
>>   set_capacity(nbd->disk, 0);
>>   nbd->flags = 0;
>>   nbd->xmit_timeout = 0;
>> + INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
>>   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
>>   del_timer_sync(>timeout_timer);
>>  }
>> @@ -804,15 +815,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
>> struct nbd_device *nbd,
>>   nbd_dev_dbg_close(nbd);
>>   kthread_stop(thread);
>>
>> - mutex_lock(>tx_lock);
>> -
>>   sock_shutdown(nbd);
>> + mutex_lock(>tx_lock);
>>   nbd_clear_que(nbd);
>>   kill_bdev(bdev);
>>   nbd_bdev_reset(bdev);
>>
>>   if 

Re: [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

Hi Markus,

On Tue, Jun 14, 2016 at 2:29 PM, Markus Pargmann  wrote:
>
> On Thursday 02 June 2016 13:25:00 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.
> >
> > Each open of a nbd device is refcounted, while
> > the userland program [nbd-client] doing the
> > NBD_DO_IT ioctl would now wait for any other users
> > of this device before invalidating the nbd device.
> >
> > Signed-off-by: Pranay Kr. Srivastava 
> > ---
> >  drivers/block/nbd.c | 58 
> > +
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index d1d898d..4da40dc 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -70,10 +70,13 @@ struct nbd_device {
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >   struct dentry *dbg_dir;
> >  #endif
> > + atomic_t inuse;
> >   /*
> >*This is specifically for calling sock_shutdown, for now.
> >*/
> >   struct work_struct ws_shutdown;
> > + struct kref users;
> > + struct completion user_completion;
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -104,6 +107,7 @@ static DEFINE_SPINLOCK(nbd_lock);
> >   * Shutdown function for nbd_dev work struct.
> >   */
> >  static void nbd_ws_func_shutdown(struct work_struct *);
> > +static void nbd_kref_release(struct kref *);
> >
> >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
> >  {
> > @@ -682,6 +686,8 @@ static void nbd_reset(struct nbd_device *nbd)
> >   nbd->flags = 0;
> >   nbd->xmit_timeout = 0;
> >   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> > + init_completion(>user_completion);
> > + kref_init(>users);
> >   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> >   del_timer_sync(>timeout_timer);
> >  }
> > @@ -815,6 +821,14 @@ static int __nbd_ioctl(struct block_device *bdev, 
> > struct nbd_device *nbd,
> >   kthread_stop(thread);
> >
> >   sock_shutdown(nbd);
> > + /*
> > +  * kref_init initializes with ref count as 1,
> > +  * nbd_client, or the user-land program executing
> > +  * this ioctl will make the refcount to 2[at least]
> > +  * so subtracting 2 from refcount.
> > +  */
> > + kref_sub(>users, 2, nbd_kref_release);
>
> Why don't you use a kref_put?

Ok, so I'll try to explain as I've understood the problem.

When the module is loaded the kref is initialized to 1.

Suppose now, someone has started nbd-client [nbdC-1] , then this
nbd-client will increase the ref count to 2. So far so good...

Now let's say this device is being shutdown via nbd-client[nbdC-2].

nbdC-1 will subtract the refcount by two, it has to do in NBD_DO_IT
since device file will not
be closed until after ioctl is over, and it'll wait_for_completion.

nbdC-2 now closes it's use of device file, this makes the refcount as
zero and completion
is triggered with nbdC-1 completed.

Now we don't want to trigger kref_put when nbdC-1 closes the device
file so kref_put needs
to be conditional in this regard so for that in_use is used.


>
> > + wait_for_completion(>user_completion);
> >   mutex_lock(>tx_lock);
> >   nbd_clear_que(nbd);
> >   kill_bdev(bdev);
> > @@ -865,13 +879,56 @@ static int nbd_ioctl(struct block_device *bdev, 
> > fmode_t mode,
> >
> >   return error;
> >  }
> > +static void nbd_kref_release(struct kref *kref_users)
> > +{
> > + struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
> > + users);
>
> Not indented to opening bracket.
>
> > + pr_debug("Releasing kref [%s]\n", __func__);
> > + atomic_set(>inuse, 0);
> > + complete(>user_completion);
> > +
> > +}
> > +
> > +static int nbd_open(struct block_device *bdev, fmode_t mode)
> > +{
> > + struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> > +
> > + if (kref_get_unless_zero(_dev->users))
> > + atomic_set(_dev->inuse, 1);
> > +
> > + pr_debug("Opening nbd_dev %s. Active users = %u\n",
> > + bdev->bd_disk->disk_name,
> > + atomic_read(_dev->users.refcount) - 1);
>
> Indent to opening bracket.
>
> > + return 0;
> > +}
> > +
> > +static void nbd_release(struct gendisk *disk, fmode_t mode)
> > +{
> > + struct nbd_device *nbd_dev = disk->private_data;
> > + /*
> > + *kref_init initializes ref count to 1, so we
> > + *we check for refcount to be 2 for a final put.
> > + *
> > + *kref needs to be re-initialized just here as the
> > + *other process holding it must see the ref 

Re: [PATCH v2 4/5]nbd: make nbd device wait for its users.

[Pranay Srivastava]

Hi Markus,

On Tue, Jun 14, 2016 at 2:29 PM, Markus Pargmann  wrote:
>
> On Thursday 02 June 2016 13:25:00 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.
> >
> > Each open of a nbd device is refcounted, while
> > the userland program [nbd-client] doing the
> > NBD_DO_IT ioctl would now wait for any other users
> > of this device before invalidating the nbd device.
> >
> > Signed-off-by: Pranay Kr. Srivastava 
> > ---
> >  drivers/block/nbd.c | 58 
> > +
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index d1d898d..4da40dc 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -70,10 +70,13 @@ struct nbd_device {
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >   struct dentry *dbg_dir;
> >  #endif
> > + atomic_t inuse;
> >   /*
> >*This is specifically for calling sock_shutdown, for now.
> >*/
> >   struct work_struct ws_shutdown;
> > + struct kref users;
> > + struct completion user_completion;
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -104,6 +107,7 @@ static DEFINE_SPINLOCK(nbd_lock);
> >   * Shutdown function for nbd_dev work struct.
> >   */
> >  static void nbd_ws_func_shutdown(struct work_struct *);
> > +static void nbd_kref_release(struct kref *);
> >
> >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
> >  {
> > @@ -682,6 +686,8 @@ static void nbd_reset(struct nbd_device *nbd)
> >   nbd->flags = 0;
> >   nbd->xmit_timeout = 0;
> >   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> > + init_completion(>user_completion);
> > + kref_init(>users);
> >   queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> >   del_timer_sync(>timeout_timer);
> >  }
> > @@ -815,6 +821,14 @@ static int __nbd_ioctl(struct block_device *bdev, 
> > struct nbd_device *nbd,
> >   kthread_stop(thread);
> >
> >   sock_shutdown(nbd);
> > + /*
> > +  * kref_init initializes with ref count as 1,
> > +  * nbd_client, or the user-land program executing
> > +  * this ioctl will make the refcount to 2[at least]
> > +  * so subtracting 2 from refcount.
> > +  */
> > + kref_sub(>users, 2, nbd_kref_release);
>
> Why don't you use a kref_put?

Ok, so I'll try to explain as I've understood the problem.

When the module is loaded the kref is initialized to 1.

Suppose now, someone has started nbd-client [nbdC-1] , then this
nbd-client will increase the ref count to 2. So far so good...

Now let's say this device is being shutdown via nbd-client[nbdC-2].

nbdC-1 will subtract the refcount by two, it has to do in NBD_DO_IT
since device file will not
be closed until after ioctl is over, and it'll wait_for_completion.

nbdC-2 now closes it's use of device file, this makes the refcount as
zero and completion
is triggered with nbdC-1 completed.

Now we don't want to trigger kref_put when nbdC-1 closes the device
file so kref_put needs
to be conditional in this regard so for that in_use is used.


>
> > + wait_for_completion(>user_completion);
> >   mutex_lock(>tx_lock);
> >   nbd_clear_que(nbd);
> >   kill_bdev(bdev);
> > @@ -865,13 +879,56 @@ static int nbd_ioctl(struct block_device *bdev, 
> > fmode_t mode,
> >
> >   return error;
> >  }
> > +static void nbd_kref_release(struct kref *kref_users)
> > +{
> > + struct nbd_device *nbd = container_of(kref_users, struct nbd_device,
> > + users);
>
> Not indented to opening bracket.
>
> > + pr_debug("Releasing kref [%s]\n", __func__);
> > + atomic_set(>inuse, 0);
> > + complete(>user_completion);
> > +
> > +}
> > +
> > +static int nbd_open(struct block_device *bdev, fmode_t mode)
> > +{
> > + struct nbd_device *nbd_dev = bdev->bd_disk->private_data;
> > +
> > + if (kref_get_unless_zero(_dev->users))
> > + atomic_set(_dev->inuse, 1);
> > +
> > + pr_debug("Opening nbd_dev %s. Active users = %u\n",
> > + bdev->bd_disk->disk_name,
> > + atomic_read(_dev->users.refcount) - 1);
>
> Indent to opening bracket.
>
> > + return 0;
> > +}
> > +
> > +static void nbd_release(struct gendisk *disk, fmode_t mode)
> > +{
> > + struct nbd_device *nbd_dev = disk->private_data;
> > + /*
> > + *kref_init initializes ref count to 1, so we
> > + *we check for refcount to be 2 for a final put.
> > + *
> > + *kref needs to be re-initialized just here as the
> > + *other process holding it must see the ref count as 2.
> > + */
> > + if 

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hello,

On Thu, Jun 9, 2016 at 3:33 PM, Pranay Srivastava <pran...@gmail.com> wrote:
>
> Hello
>
>
> On Thu, Jun 2, 2016 at 3:54 PM, Pranay Kr. Srivastava <pran...@gmail.com> 
> wrote:
> > spinlocked ranges should be small and not contain calls into huge
> > subfunctions. Fix my mistake and just get the pointer to the socket
> > instead of doing everything with spinlock held.
> >
> > Reported-by: Mikulas Patocka <miku...@twibright.com>
> > Signed-off-by: Markus Pargmann <m...@pengutronix.de>
> >
> > Changelog:
> > Pranay Kr. Srivastava<pran...@gmail.com>:
> >
> > 1) Use spin_lock instead of irq version for sock_shutdown.
> >
> > 2) Use system work queue to actually trigger the shutdown of
> >socket. This solves the issue when kernel_sendmsg is currently
> >blocked while a timeout occurs.
> >
> > Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
> > ---
> >  drivers/block/nbd.c | 65 
> > ++---
> >  1 file changed, 42 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 31e73a7..0339d40 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -39,6 +39,7 @@
> >  #include 
> >
> >  #include 
> > +#include 
> >
> >  struct nbd_device {
> > u32 flags;
> > @@ -69,6 +70,10 @@ struct nbd_device {
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > struct dentry *dbg_dir;
> >  #endif
> > +   /*
> > +*This is specifically for calling sock_shutdown, for now.
> > +*/
> > +   struct work_struct ws_shutdown;
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -95,6 +100,11 @@ static int max_part;
> >   */
> >  static DEFINE_SPINLOCK(nbd_lock);
> >
> > +/*
> > + * Shutdown function for nbd_dev work struct.
> > + */
> > +static void nbd_ws_func_shutdown(struct work_struct *);
> > +
> >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
> >  {
> > return disk_to_dev(nbd->disk);
> > @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> > struct request *req)
> >   */
> >  static void sock_shutdown(struct nbd_device *nbd)
> >  {
> > -   spin_lock_irq(>sock_lock);
> > -
> > -   if (!nbd->sock) {
> > -   spin_unlock_irq(>sock_lock);
> > -   return;
> > -   }
> > +   struct socket *sock;
> >
> > -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> > -   sockfd_put(nbd->sock);
> > +   spin_lock(>sock_lock);
> > +   sock = nbd->sock;
> > nbd->sock = NULL;
> > -   spin_unlock_irq(>sock_lock);
> > +   spin_unlock(>sock_lock);
> > +
> > +   if (!sock)
> > +   return;
> >
> > del_timer(>timeout_timer);
> > +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> > +   kernel_sock_shutdown(sock, SHUT_RDWR);
> > +   sockfd_put(sock);
> >  }
> >
> >  static void nbd_xmit_timeout(unsigned long arg)
> >  {
> > struct nbd_device *nbd = (struct nbd_device *)arg;
> > -   unsigned long flags;
> >
> > if (list_empty(>queue_head))
> > return;
> > -
> > -   spin_lock_irqsave(>sock_lock, flags);
> > -
> > nbd->timedout = true;
> > -
> > -   if (nbd->sock)
> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> > -
> > -   spin_unlock_irqrestore(>sock_lock, flags);
> > -
> > +   schedule_work(>ws_shutdown);
> > +   /*
> > +* Make sure sender thread sees nbd->timedout.
> > +*/
> > +   smp_wmb();
> > +   wake_up(>waiting_wq);
> > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> > connection\n");
> >  }
> >
> > @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
> > spin_unlock_irq(>queue_lock);
> >
> > /* handle request */
> > -   nbd_handle_req(nbd, req);
> > +   if (nbd->timedout) {
> > +   req->errors++;
> > +   nbd_end_request(nbd,

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hello,

On Thu, Jun 9, 2016 at 3:33 PM, Pranay Srivastava  wrote:
>
> Hello
>
>
> On Thu, Jun 2, 2016 at 3:54 PM, Pranay Kr. Srivastava  
> wrote:
> > spinlocked ranges should be small and not contain calls into huge
> > subfunctions. Fix my mistake and just get the pointer to the socket
> > instead of doing everything with spinlock held.
> >
> > Reported-by: Mikulas Patocka 
> > Signed-off-by: Markus Pargmann 
> >
> > Changelog:
> > Pranay Kr. Srivastava:
> >
> > 1) Use spin_lock instead of irq version for sock_shutdown.
> >
> > 2) Use system work queue to actually trigger the shutdown of
> >socket. This solves the issue when kernel_sendmsg is currently
> >blocked while a timeout occurs.
> >
> > Signed-off-by: Pranay Kr. Srivastava 
> > ---
> >  drivers/block/nbd.c | 65 
> > ++---
> >  1 file changed, 42 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 31e73a7..0339d40 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -39,6 +39,7 @@
> >  #include 
> >
> >  #include 
> > +#include 
> >
> >  struct nbd_device {
> > u32 flags;
> > @@ -69,6 +70,10 @@ struct nbd_device {
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > struct dentry *dbg_dir;
> >  #endif
> > +   /*
> > +*This is specifically for calling sock_shutdown, for now.
> > +*/
> > +   struct work_struct ws_shutdown;
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -95,6 +100,11 @@ static int max_part;
> >   */
> >  static DEFINE_SPINLOCK(nbd_lock);
> >
> > +/*
> > + * Shutdown function for nbd_dev work struct.
> > + */
> > +static void nbd_ws_func_shutdown(struct work_struct *);
> > +
> >  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
> >  {
> > return disk_to_dev(nbd->disk);
> > @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> > struct request *req)
> >   */
> >  static void sock_shutdown(struct nbd_device *nbd)
> >  {
> > -   spin_lock_irq(>sock_lock);
> > -
> > -   if (!nbd->sock) {
> > -   spin_unlock_irq(>sock_lock);
> > -   return;
> > -   }
> > +   struct socket *sock;
> >
> > -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> > -   sockfd_put(nbd->sock);
> > +   spin_lock(>sock_lock);
> > +   sock = nbd->sock;
> > nbd->sock = NULL;
> > -   spin_unlock_irq(>sock_lock);
> > +   spin_unlock(>sock_lock);
> > +
> > +   if (!sock)
> > +   return;
> >
> > del_timer(>timeout_timer);
> > +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> > +   kernel_sock_shutdown(sock, SHUT_RDWR);
> > +   sockfd_put(sock);
> >  }
> >
> >  static void nbd_xmit_timeout(unsigned long arg)
> >  {
> > struct nbd_device *nbd = (struct nbd_device *)arg;
> > -   unsigned long flags;
> >
> > if (list_empty(>queue_head))
> > return;
> > -
> > -   spin_lock_irqsave(>sock_lock, flags);
> > -
> > nbd->timedout = true;
> > -
> > -   if (nbd->sock)
> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> > -
> > -   spin_unlock_irqrestore(>sock_lock, flags);
> > -
> > +   schedule_work(>ws_shutdown);
> > +   /*
> > +* Make sure sender thread sees nbd->timedout.
> > +*/
> > +   smp_wmb();
> > +   wake_up(>waiting_wq);
> > dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> > connection\n");
> >  }
> >
> > @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
> > spin_unlock_irq(>queue_lock);
> >
> > /* handle request */
> > -   nbd_handle_req(nbd, req);
> > +   if (nbd->timedout) {
> > +   req->errors++;
> > +   nbd_end_request(nbd, req);
> > +   } else
> > +   nbd_handle_req(nbd, req);
> > }
> >
> > nbd->task_send = NU

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hello


On Thu, Jun 2, 2016 at 3:54 PM, Pranay Kr. Srivastava  wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 65 
> ++---
>  1 file changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..0339d40 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
> spin_unlock_irq(>queue_lock);
>
> /* handle request */
> -   nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -672,6 +682,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -804,15 +815,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
>
> -   mutex_lock(>tx_lock);
> -
> sock_shutdown(nbd);
> +   mutex_lock(>tx_lock);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
>
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> +
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> @@ -863,6 +874,14 @@ static const struct block_device_operations nbd_fops =
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void 

Re: [PATCH v2 1/5] nbd: fix might_sleep warning on socket shutdown.

[Pranay Srivastava]

Hello


On Thu, Jun 2, 2016 at 3:54 PM, Pranay Kr. Srivastava  wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
>
> Changelog:
> Pranay Kr. Srivastava:
>
> 1) Use spin_lock instead of irq version for sock_shutdown.
>
> 2) Use system work queue to actually trigger the shutdown of
>socket. This solves the issue when kernel_sendmsg is currently
>blocked while a timeout occurs.
>
> Signed-off-by: Pranay Kr. Srivastava 
> ---
>  drivers/block/nbd.c | 65 
> ++---
>  1 file changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..0339d40 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -39,6 +39,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  struct nbd_device {
> u32 flags;
> @@ -69,6 +70,10 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> struct dentry *dbg_dir;
>  #endif
> +   /*
> +*This is specifically for calling sock_shutdown, for now.
> +*/
> +   struct work_struct ws_shutdown;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -95,6 +100,11 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>
> +/*
> + * Shutdown function for nbd_dev work struct.
> + */
> +static void nbd_ws_func_shutdown(struct work_struct *);
> +
>  static inline struct device *nbd_to_dev(struct nbd_device *nbd)
>  {
> return disk_to_dev(nbd->disk);
> @@ -172,39 +182,35 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> -   return;
> -   }
> +   struct socket *sock;
>
> -   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> +   spin_lock(>sock_lock);
> +   sock = nbd->sock;
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   spin_unlock(>sock_lock);
> +
> +   if (!sock)
> +   return;
>
> del_timer(>timeout_timer);
> +   dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
> -
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> -
> +   schedule_work(>ws_shutdown);
> +   /*
> +* Make sure sender thread sees nbd->timedout.
> +*/
> +   smp_wmb();
> +   wake_up(>waiting_wq);
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
>
> @@ -592,7 +598,11 @@ static int nbd_thread_send(void *data)
> spin_unlock_irq(>queue_lock);
>
> /* handle request */
> -   nbd_handle_req(nbd, req);
> +   if (nbd->timedout) {
> +   req->errors++;
> +   nbd_end_request(nbd, req);
> +   } else
> +   nbd_handle_req(nbd, req);
> }
>
> nbd->task_send = NULL;
> @@ -672,6 +682,7 @@ static void nbd_reset(struct nbd_device *nbd)
> set_capacity(nbd->disk, 0);
> nbd->flags = 0;
> nbd->xmit_timeout = 0;
> +   INIT_WORK(>ws_shutdown, nbd_ws_func_shutdown);
> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
> del_timer_sync(>timeout_timer);
>  }
> @@ -804,15 +815,15 @@ static int __nbd_ioctl(struct block_device *bdev, 
> struct nbd_device *nbd,
> nbd_dev_dbg_close(nbd);
> kthread_stop(thread);
>
> -   mutex_lock(>tx_lock);
> -
> sock_shutdown(nbd);
> +   mutex_lock(>tx_lock);
> nbd_clear_que(nbd);
> kill_bdev(bdev);
> nbd_bdev_reset(bdev);
>
> if (nbd->disconnect) /* user requested, ignore socket errors 
> */
> error = 0;
> +
> if (nbd->timedout)
> error = -ETIMEDOUT;
>
> @@ -863,6 +874,14 @@ static const struct block_device_operations nbd_fops =
> .compat_ioctl = nbd_ioctl,
>  };
>
> +static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> +{
> +   struct nbd_device *nbd_dev = container_of(ws_nbd, 

Re: [PATCH 0/4]nbd: fixes for nbd

[Pranay Srivastava]

This got rejected by mailing-list[due to HTML content] so resending it again.

On Sat, Jun 4, 2016 at 10:25 AM, Pranay Srivastava <pran...@gmail.com> wrote:
>
>
> On Thursday, June 2, 2016, Pranay Kr. Srivastava <pran...@gmail.com> wrote:
>> This patch series fixes the following
>>
>> 1) fix might_sleep warning on socket shutdown:
>>Fix sock_shutdown to avoid calling kernel_sock_shutdown
>>while holding spin_lock.
>>
>> 2) cleanup nbd_set_socket
>>Cleanup nbd_set_socket to use spin_lock instead of
>>irq version and remove the goto statement in favour
>>of a simple if-else statement.
>>
>> 3) fix various coding standard warnings
>>Make shutdown get called in a process context instead, using
>>system_wq.
>>
>> 4) make nbd device wait for its users.
>>When a timeout or error occurs then nbd driver simply kills
>>the block device. Many filesystem(s) example ext2/ext3 don't
>>expect their buffer heads to disappear like that. Fix this
>>by making nbd device wait for its users.
>>
>>Introduced a new field to check if the device is currently
>>in use or not. This helps to check if the kref_put should
>>be done on device release or not.
>>
>>This field needs to be atomic as the release function may
>>be called from NBD_DO_IT as well as from device's release
>>function.
>>
>> 5) use device_attr macros for sysfs attribute
>>use DEVICE_ATTR_RO for sysfs pid attribute.
>>
>> Changelog for v2:
>> 1) fix might_sleep warning on socket shutdown
>>use bool timedout instead of atomic
>>
>> 2) cleanup nbd_set_socket
>>Added this new patch to this series.
>>
>> 3) fix various coding standard warnings
>>No Change.
>>
>> 4) make nbd device wait for its users
>>Earlier version used to do a final kref put when
>>the kref->counter == 2. This required a check of
>>the internal atomic counter of kref which was ugly.
>>
>>v2 of this patch make this more readable and doesn't
>>do manual check of the internal counter used by kref.
>>
>> 5) use device_attr macros for sysfs attribute
>>No Change.
>>
>> Pranay Kr. Srivastava (5):
>>   fix might_sleep warning on socket shutdown.
>>   cleanup nbd_set_socket
>>   fix various coding standard warnings
>>   make nbd device wait for its users.
>>   use device_attr macros for sysfs attribute
>>
>>  drivers/block/nbd.c | 173
>> +---
>>  1 file changed, 124 insertions(+), 49 deletions(-)
>>
>> --
>> 2.6.2
>>
>>
> Markus can you please review this series.
>
> --
> ---P.K.S
>

Can anyone kindly review this series?


-- 
---P.K.S

Re: [PATCH 0/4]nbd: fixes for nbd

[Pranay Srivastava]

This got rejected by mailing-list[due to HTML content] so resending it again.

On Sat, Jun 4, 2016 at 10:25 AM, Pranay Srivastava  wrote:
>
>
> On Thursday, June 2, 2016, Pranay Kr. Srivastava  wrote:
>> This patch series fixes the following
>>
>> 1) fix might_sleep warning on socket shutdown:
>>Fix sock_shutdown to avoid calling kernel_sock_shutdown
>>while holding spin_lock.
>>
>> 2) cleanup nbd_set_socket
>>Cleanup nbd_set_socket to use spin_lock instead of
>>irq version and remove the goto statement in favour
>>of a simple if-else statement.
>>
>> 3) fix various coding standard warnings
>>Make shutdown get called in a process context instead, using
>>system_wq.
>>
>> 4) make nbd device wait for its users.
>>When a timeout or error occurs then nbd driver simply kills
>>the block device. Many filesystem(s) example ext2/ext3 don't
>>expect their buffer heads to disappear like that. Fix this
>>by making nbd device wait for its users.
>>
>>Introduced a new field to check if the device is currently
>>in use or not. This helps to check if the kref_put should
>>be done on device release or not.
>>
>>This field needs to be atomic as the release function may
>>be called from NBD_DO_IT as well as from device's release
>>function.
>>
>> 5) use device_attr macros for sysfs attribute
>>use DEVICE_ATTR_RO for sysfs pid attribute.
>>
>> Changelog for v2:
>> 1) fix might_sleep warning on socket shutdown
>>use bool timedout instead of atomic
>>
>> 2) cleanup nbd_set_socket
>>Added this new patch to this series.
>>
>> 3) fix various coding standard warnings
>>No Change.
>>
>> 4) make nbd device wait for its users
>>Earlier version used to do a final kref put when
>>the kref->counter == 2. This required a check of
>>the internal atomic counter of kref which was ugly.
>>
>>v2 of this patch make this more readable and doesn't
>>do manual check of the internal counter used by kref.
>>
>> 5) use device_attr macros for sysfs attribute
>>No Change.
>>
>> Pranay Kr. Srivastava (5):
>>   fix might_sleep warning on socket shutdown.
>>   cleanup nbd_set_socket
>>   fix various coding standard warnings
>>   make nbd device wait for its users.
>>   use device_attr macros for sysfs attribute
>>
>>  drivers/block/nbd.c | 173
>> +---
>>  1 file changed, 124 insertions(+), 49 deletions(-)
>>
>> --
>> 2.6.2
>>
>>
> Markus can you please review this series.
>
> --
> ---P.K.S
>

Can anyone kindly review this series?


-- 
---P.K.S

Re: [PATCH 0/4]nbd: fixes for nbd

[Pranay Srivastava]

Hi

On Tue, May 24, 2016 at 4:56 PM, Pranay Kr. Srivastava
 wrote:
> This patch series fixes the following
>
> 1) fix might_sleep warning on socket shutdown:
>Fix sock_shutdown to avoid calling kernel_sock_shutdown
>while holding spin_lock.
>
> 2)fix various coding standard warnings
>Make shutdown get called in a process context instead, using
>system_wq.
>
> 3) make nbd device wait for its users.
>When a timeout or error occurs then nbd driver simply kills
>the block device. Many filesystem(s) example ext2/ext3 don't
>expect their buffer heads to disappear like that. Fix this
>by making nbd device wait for its users.
>
> 4) use device_attr macros for sysfs attribute
>use DEVICE_ATTR_RO for sysfs pid attribute.
>
> Pranay Kr. Srivastava (4):
>   fix might_sleep warning on socket shutdown.
>   fix various coding standard warnings
>   make nbd device wait for its users.
>   use device_attr macros for sysfs attribute
>
>  drivers/block/nbd.c |  150 
> +++
>  1 file changed, 105 insertions(+), 45 deletions(-)
>
> --
> 1.7.9.5
>

Can anyone review this?

-- 
---P.K.S

Re: [PATCH 0/4]nbd: fixes for nbd

[Pranay Srivastava]

Hi

On Tue, May 24, 2016 at 4:56 PM, Pranay Kr. Srivastava
 wrote:
> This patch series fixes the following
>
> 1) fix might_sleep warning on socket shutdown:
>Fix sock_shutdown to avoid calling kernel_sock_shutdown
>while holding spin_lock.
>
> 2)fix various coding standard warnings
>Make shutdown get called in a process context instead, using
>system_wq.
>
> 3) make nbd device wait for its users.
>When a timeout or error occurs then nbd driver simply kills
>the block device. Many filesystem(s) example ext2/ext3 don't
>expect their buffer heads to disappear like that. Fix this
>by making nbd device wait for its users.
>
> 4) use device_attr macros for sysfs attribute
>use DEVICE_ATTR_RO for sysfs pid attribute.
>
> Pranay Kr. Srivastava (4):
>   fix might_sleep warning on socket shutdown.
>   fix various coding standard warnings
>   make nbd device wait for its users.
>   use device_attr macros for sysfs attribute
>
>  drivers/block/nbd.c |  150 
> +++
>  1 file changed, 105 insertions(+), 45 deletions(-)
>
> --
> 1.7.9.5
>

Can anyone review this?

-- 
---P.K.S

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Mon, May 23, 2016 at 4:02 PM, Pranay Srivastava <pran...@gmail.com> wrote:
> Hi Markus
>
> On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann <m...@pengutronix.de> wrote:
>> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <m...@pengutronix.de> 
>>> wrote:
>>> > Hi,
>>> >
>>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>>> >> This patch fixes the warning generated when a timeout occurs
>>> >> on the request and socket is closed from a non-sleep context
>>> >> by
>>> >>
>>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>>> >
>>> > What happens if a send blocks?
>>>
>>> socket closing needs to be moved to a non-atomic context and,
>>> sender thread seemed to be a good place to do this. If you mean
>>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>>> then yes I think that should be removed. I need to re-check how nbd-server
>>> behaves in that case.
>>
>> No that's not what I meant. Your approach uses the sender thread as a
>> worker to close the socket. You are using waiting_wq to notify the
>> sender thread. That's fine so far.
>>
>> But what happens if the sender thread is at this point trying to send on
>> the socket which blocks? Then the timeout triggers and waiting_wq will
>> notify the sending thread as soon as it left the sending routine. But it
>> will not interrupt the thread that is waiting in kernel_sendmsg() and
>> the sending thread will be stuck much longer than specified in the
>> timeout.
>
> So socket shutdown must be triggered immediately. I've done a version using
> system_wq for this and appears to be good. I'll be sending that soon after 
> doing
> cleanup and applying your sock_shutdown patch you sent earlier.
>
>>
>>>
>>> >
>>> >>
>>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>>> >>nbd_xmit_timeout doesn't need to hold it anymore.
>>> >
>>> > I can't see why we need a mutex instead of a spinlock?
>>>
>>> you are right, with your earlier patch we don't need it to be a mutex.
>>>
>>> >
>>> >>
>>> >> Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>>> >> ---
>>> >>  drivers/block/nbd.c | 65 
>>> >> -
>>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>>> >>
>>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>> >> index 31e73a7..c79bcd7 100644
>>> >> --- a/drivers/block/nbd.c
>>> >> +++ b/drivers/block/nbd.c
>>> >> @@ -57,12 +57,12 @@ struct nbd_device {
>>> >>   int blksize;
>>> >>   loff_t bytesize;
>>> >>   int xmit_timeout;
>>> >> - bool timedout;
>>> >> + atomic_t timedout;
>>> >>   bool disconnect; /* a disconnect has been requested by user */
>>> >>
>>> >>   struct timer_list timeout_timer;
>>> >>   /* protects initialization and shutdown of the socket */
>>> >> - spinlock_t sock_lock;
>>> >> + struct mutex sock_lock;
>>> >>   struct task_struct *task_recv;
>>> >>   struct task_struct *task_send;
>>> >>
>>> >> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>>> >> struct request *req)
>>> >>   */
>>> >>  static void sock_shutdown(struct nbd_device *nbd)
>>> >>  {
>>> >> - spin_lock_irq(>sock_lock);
>>> >> -
>>> >> + mutex_lock(>sock_lock);
>>> >>   if (!nbd->sock) {
>>> >> - spin_unlock_irq(>sock_lock);
>>> >> + mutex_unlock(>sock_lock);
>>> >>   return;
>>> >>   }
>>> >>
>>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>> >>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>> >>   sockfd_put(nbd->sock);
>>> >>   nbd->sock = NULL;
>>> >> - spin_unlock_irq(>sock_lock);
>>> >> -
>>> 

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Mon, May 23, 2016 at 4:02 PM, Pranay Srivastava  wrote:
> Hi Markus
>
> On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann  wrote:
>> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann  
>>> wrote:
>>> > Hi,
>>> >
>>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>>> >> This patch fixes the warning generated when a timeout occurs
>>> >> on the request and socket is closed from a non-sleep context
>>> >> by
>>> >>
>>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>>> >
>>> > What happens if a send blocks?
>>>
>>> socket closing needs to be moved to a non-atomic context and,
>>> sender thread seemed to be a good place to do this. If you mean
>>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>>> then yes I think that should be removed. I need to re-check how nbd-server
>>> behaves in that case.
>>
>> No that's not what I meant. Your approach uses the sender thread as a
>> worker to close the socket. You are using waiting_wq to notify the
>> sender thread. That's fine so far.
>>
>> But what happens if the sender thread is at this point trying to send on
>> the socket which blocks? Then the timeout triggers and waiting_wq will
>> notify the sending thread as soon as it left the sending routine. But it
>> will not interrupt the thread that is waiting in kernel_sendmsg() and
>> the sending thread will be stuck much longer than specified in the
>> timeout.
>
> So socket shutdown must be triggered immediately. I've done a version using
> system_wq for this and appears to be good. I'll be sending that soon after 
> doing
> cleanup and applying your sock_shutdown patch you sent earlier.
>
>>
>>>
>>> >
>>> >>
>>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>>> >>nbd_xmit_timeout doesn't need to hold it anymore.
>>> >
>>> > I can't see why we need a mutex instead of a spinlock?
>>>
>>> you are right, with your earlier patch we don't need it to be a mutex.
>>>
>>> >
>>> >>
>>> >> Signed-off-by: Pranay Kr. Srivastava 
>>> >> ---
>>> >>  drivers/block/nbd.c | 65 
>>> >> -
>>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>>> >>
>>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>> >> index 31e73a7..c79bcd7 100644
>>> >> --- a/drivers/block/nbd.c
>>> >> +++ b/drivers/block/nbd.c
>>> >> @@ -57,12 +57,12 @@ struct nbd_device {
>>> >>   int blksize;
>>> >>   loff_t bytesize;
>>> >>   int xmit_timeout;
>>> >> - bool timedout;
>>> >> + atomic_t timedout;
>>> >>   bool disconnect; /* a disconnect has been requested by user */
>>> >>
>>> >>   struct timer_list timeout_timer;
>>> >>   /* protects initialization and shutdown of the socket */
>>> >> - spinlock_t sock_lock;
>>> >> + struct mutex sock_lock;
>>> >>   struct task_struct *task_recv;
>>> >>   struct task_struct *task_send;
>>> >>
>>> >> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>>> >> struct request *req)
>>> >>   */
>>> >>  static void sock_shutdown(struct nbd_device *nbd)
>>> >>  {
>>> >> - spin_lock_irq(>sock_lock);
>>> >> -
>>> >> + mutex_lock(>sock_lock);
>>> >>   if (!nbd->sock) {
>>> >> - spin_unlock_irq(>sock_lock);
>>> >> + mutex_unlock(>sock_lock);
>>> >>   return;
>>> >>   }
>>> >>
>>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>> >>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>> >>   sockfd_put(nbd->sock);
>>> >>   nbd->sock = NULL;
>>> >> - spin_unlock_irq(>sock_lock);
>>> >> -
>>> >> + mutex_unlock(>sock_lock);
>>> >>   del_timer(>timeout_timer);
&g

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

Hi Markus

On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann <m...@pengutronix.de> wrote:
> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann <m...@pengutronix.de> 
>> wrote:
>> > Hi,
>> >
>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> >> This patch fixes the warning generated when a timeout occurs
>> >> on the request and socket is closed from a non-sleep context
>> >> by
>> >>
>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > What happens if a send blocks?
>>
>> socket closing needs to be moved to a non-atomic context and,
>> sender thread seemed to be a good place to do this. If you mean
>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>> then yes I think that should be removed. I need to re-check how nbd-server
>> behaves in that case.
>
> No that's not what I meant. Your approach uses the sender thread as a
> worker to close the socket. You are using waiting_wq to notify the
> sender thread. That's fine so far.
>
> But what happens if the sender thread is at this point trying to send on
> the socket which blocks? Then the timeout triggers and waiting_wq will
> notify the sending thread as soon as it left the sending routine. But it
> will not interrupt the thread that is waiting in kernel_sendmsg() and
> the sending thread will be stuck much longer than specified in the
> timeout.

So socket shutdown must be triggered immediately. I've done a version using
system_wq for this and appears to be good. I'll be sending that soon after doing
cleanup and applying your sock_shutdown patch you sent earlier.

>
>>
>> >
>> >>
>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>> >>nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > I can't see why we need a mutex instead of a spinlock?
>>
>> you are right, with your earlier patch we don't need it to be a mutex.
>>
>> >
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava <pran...@gmail.com>
>> >> ---
>> >>  drivers/block/nbd.c | 65 
>> >> -
>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index 31e73a7..c79bcd7 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -57,12 +57,12 @@ struct nbd_device {
>> >>   int blksize;
>> >>   loff_t bytesize;
>> >>   int xmit_timeout;
>> >> - bool timedout;
>> >> + atomic_t timedout;
>> >>   bool disconnect; /* a disconnect has been requested by user */
>> >>
>> >>   struct timer_list timeout_timer;
>> >>   /* protects initialization and shutdown of the socket */
>> >> - spinlock_t sock_lock;
>> >> + struct mutex sock_lock;
>> >>   struct task_struct *task_recv;
>> >>   struct task_struct *task_send;
>> >>
>> >> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> >> struct request *req)
>> >>   */
>> >>  static void sock_shutdown(struct nbd_device *nbd)
>> >>  {
>> >> - spin_lock_irq(>sock_lock);
>> >> -
>> >> + mutex_lock(>sock_lock);
>> >>   if (!nbd->sock) {
>> >> - spin_unlock_irq(>sock_lock);
>> >> + mutex_unlock(>sock_lock);
>> >>   return;
>> >>   }
>> >>
>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>> >>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> >>   sockfd_put(nbd->sock);
>> >>   nbd->sock = NULL;
>> >> - spin_unlock_irq(>sock_lock);
>> >> -
>> >> + mutex_unlock(>sock_lock);
>> >>   del_timer(>timeout_timer);
>> >>  }
>> >>
>> >>  static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >> - unsigned long flags;
>> >>
>> >>   if (list_empty(>queue_head))
>> >>   return;
>> >>
>> >> 

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

Hi Markus

On Fri, May 20, 2016 at 1:52 PM, Markus Pargmann  wrote:
> On Friday 20 May 2016 02:05:36 Pranay Srivastava wrote:
>> On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann  
>> wrote:
>> > Hi,
>> >
>> > On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> >> This patch fixes the warning generated when a timeout occurs
>> >> on the request and socket is closed from a non-sleep context
>> >> by
>> >>
>> >> 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > What happens if a send blocks?
>>
>> socket closing needs to be moved to a non-atomic context and,
>> sender thread seemed to be a good place to do this. If you mean
>> send blocks just before calling sock_shutdown[?] in nbd_thread_send
>> then yes I think that should be removed. I need to re-check how nbd-server
>> behaves in that case.
>
> No that's not what I meant. Your approach uses the sender thread as a
> worker to close the socket. You are using waiting_wq to notify the
> sender thread. That's fine so far.
>
> But what happens if the sender thread is at this point trying to send on
> the socket which blocks? Then the timeout triggers and waiting_wq will
> notify the sending thread as soon as it left the sending routine. But it
> will not interrupt the thread that is waiting in kernel_sendmsg() and
> the sending thread will be stuck much longer than specified in the
> timeout.

So socket shutdown must be triggered immediately. I've done a version using
system_wq for this and appears to be good. I'll be sending that soon after doing
cleanup and applying your sock_shutdown patch you sent earlier.

>
>>
>> >
>> >>
>> >> 2. Make sock lock to be a mutex instead of a spin lock, since
>> >>nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > I can't see why we need a mutex instead of a spinlock?
>>
>> you are right, with your earlier patch we don't need it to be a mutex.
>>
>> >
>> >>
>> >> Signed-off-by: Pranay Kr. Srivastava 
>> >> ---
>> >>  drivers/block/nbd.c | 65 
>> >> -
>> >>  1 file changed, 39 insertions(+), 26 deletions(-)
>> >>
>> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> >> index 31e73a7..c79bcd7 100644
>> >> --- a/drivers/block/nbd.c
>> >> +++ b/drivers/block/nbd.c
>> >> @@ -57,12 +57,12 @@ struct nbd_device {
>> >>   int blksize;
>> >>   loff_t bytesize;
>> >>   int xmit_timeout;
>> >> - bool timedout;
>> >> + atomic_t timedout;
>> >>   bool disconnect; /* a disconnect has been requested by user */
>> >>
>> >>   struct timer_list timeout_timer;
>> >>   /* protects initialization and shutdown of the socket */
>> >> - spinlock_t sock_lock;
>> >> + struct mutex sock_lock;
>> >>   struct task_struct *task_recv;
>> >>   struct task_struct *task_send;
>> >>
>> >> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> >> struct request *req)
>> >>   */
>> >>  static void sock_shutdown(struct nbd_device *nbd)
>> >>  {
>> >> - spin_lock_irq(>sock_lock);
>> >> -
>> >> + mutex_lock(>sock_lock);
>> >>   if (!nbd->sock) {
>> >> - spin_unlock_irq(>sock_lock);
>> >> + mutex_unlock(>sock_lock);
>> >>   return;
>> >>   }
>> >>
>> >> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>> >>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> >>   sockfd_put(nbd->sock);
>> >>   nbd->sock = NULL;
>> >> - spin_unlock_irq(>sock_lock);
>> >> -
>> >> + mutex_unlock(>sock_lock);
>> >>   del_timer(>timeout_timer);
>> >>  }
>> >>
>> >>  static void nbd_xmit_timeout(unsigned long arg)
>> >>  {
>> >>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> >> - unsigned long flags;
>> >>
>> >>   if (list_empty(>queue_head))
>> >>   return;
>> >>
>> >> - spin_lock_irqsave(>sock_lock, flags);
>> >> -
>> &

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann  wrote:
> Hi,
>
> On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> This patch fixes the warning generated when a timeout occurs
>> on the request and socket is closed from a non-sleep context
>> by
>>
>> 1. Moving the socket closing on a timeout to nbd_thread_send
>
> What happens if a send blocks?

socket closing needs to be moved to a non-atomic context and,
sender thread seemed to be a good place to do this. If you mean
send blocks just before calling sock_shutdown[?] in nbd_thread_send
then yes I think that should be removed. I need to re-check how nbd-server
behaves in that case.

>
>>
>> 2. Make sock lock to be a mutex instead of a spin lock, since
>>nbd_xmit_timeout doesn't need to hold it anymore.
>
> I can't see why we need a mutex instead of a spinlock?

you are right, with your earlier patch we don't need it to be a mutex.

>
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 65 
>> -
>>  1 file changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 31e73a7..c79bcd7 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -57,12 +57,12 @@ struct nbd_device {
>>   int blksize;
>>   loff_t bytesize;
>>   int xmit_timeout;
>> - bool timedout;
>> + atomic_t timedout;
>>   bool disconnect; /* a disconnect has been requested by user */
>>
>>   struct timer_list timeout_timer;
>>   /* protects initialization and shutdown of the socket */
>> - spinlock_t sock_lock;
>> + struct mutex sock_lock;
>>   struct task_struct *task_recv;
>>   struct task_struct *task_send;
>>
>> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> struct request *req)
>>   */
>>  static void sock_shutdown(struct nbd_device *nbd)
>>  {
>> - spin_lock_irq(>sock_lock);
>> -
>> + mutex_lock(>sock_lock);
>>   if (!nbd->sock) {
>> - spin_unlock_irq(>sock_lock);
>> + mutex_unlock(>sock_lock);
>>   return;
>>   }
>>
>> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>   sockfd_put(nbd->sock);
>>   nbd->sock = NULL;
>> - spin_unlock_irq(>sock_lock);
>> -
>> + mutex_unlock(>sock_lock);
>>   del_timer(>timeout_timer);
>>  }
>>
>>  static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> - unsigned long flags;
>>
>>   if (list_empty(>queue_head))
>>   return;
>>
>> - spin_lock_irqsave(>sock_lock, flags);
>> -
>> - nbd->timedout = true;
>> -
>> - if (nbd->sock)
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> -
>> - spin_unlock_irqrestore(>sock_lock, flags);
>> + atomic_inc(>timedout);
>> + wake_up(>waiting_wq);
>>
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>>   /* wait for something to do */
>>   wait_event_interruptible(nbd->waiting_wq,
>>kthread_should_stop() ||
>> -  !list_empty(>waiting_queue));
>> +  !list_empty(>waiting_queue) ||
>> +  atomic_read(>timedout));
>> +
>> + if (atomic_read(>timedout)) {
>> + mutex_lock(>sock_lock);
>> + if (nbd->sock) {
>> + struct request sreq;
>> +
>> + blk_rq_init(NULL, );
>> + sreq.cmd_type = REQ_TYPE_DRV_PRIV;
>> + mutex_lock(>tx_lock);
>> + nbd->disconnect = true;
>> + nbd_send_req(nbd, );
>> + mutex_unlock(>tx_lock);
>> + dev_err(disk_to_dev(nbd->disk),
>> + "Device Timeout occured.Shutting down"
>> + " socket.");
>> + }
>> + mutex_unlock(>sock_lock);
>> + sock_shutdown(nbd);
>
> Why are you trying to send something on a connection that timed out
> (nbd_send_req())? And afterwards you execute a socket shutdown so in most
> timeout cases this won't reach the server and we risk a blocking send on
> a timedout connection.

Ok. I get it. But shouldn't the server side also close it's socket as
well? I don't
think the timeout value is propagated to server or like server can
"ping" to check
if client is there right?

I agree on nbd_send_req in timedout, it shouldn't be there, just a
sock_shutdown should
do. Can you confirm 

Re: [PATCH v4 01/18] nbd: Fix might_sleep warning on xmit timeout

[Pranay Srivastava]

On Thu, May 19, 2016 at 11:52 AM, Markus Pargmann  wrote:
> Hi,
>
> On Wed, May 11, 2016 at 11:18:29AM +0300, Pranay Kr. Srivastava wrote:
>> This patch fixes the warning generated when a timeout occurs
>> on the request and socket is closed from a non-sleep context
>> by
>>
>> 1. Moving the socket closing on a timeout to nbd_thread_send
>
> What happens if a send blocks?

socket closing needs to be moved to a non-atomic context and,
sender thread seemed to be a good place to do this. If you mean
send blocks just before calling sock_shutdown[?] in nbd_thread_send
then yes I think that should be removed. I need to re-check how nbd-server
behaves in that case.

>
>>
>> 2. Make sock lock to be a mutex instead of a spin lock, since
>>nbd_xmit_timeout doesn't need to hold it anymore.
>
> I can't see why we need a mutex instead of a spinlock?

you are right, with your earlier patch we don't need it to be a mutex.

>
>>
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 65 
>> -
>>  1 file changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index 31e73a7..c79bcd7 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -57,12 +57,12 @@ struct nbd_device {
>>   int blksize;
>>   loff_t bytesize;
>>   int xmit_timeout;
>> - bool timedout;
>> + atomic_t timedout;
>>   bool disconnect; /* a disconnect has been requested by user */
>>
>>   struct timer_list timeout_timer;
>>   /* protects initialization and shutdown of the socket */
>> - spinlock_t sock_lock;
>> + struct mutex sock_lock;
>>   struct task_struct *task_recv;
>>   struct task_struct *task_send;
>>
>> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> struct request *req)
>>   */
>>  static void sock_shutdown(struct nbd_device *nbd)
>>  {
>> - spin_lock_irq(>sock_lock);
>> -
>> + mutex_lock(>sock_lock);
>>   if (!nbd->sock) {
>> - spin_unlock_irq(>sock_lock);
>> + mutex_unlock(>sock_lock);
>>   return;
>>   }
>>
>> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
>>   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>>   sockfd_put(nbd->sock);
>>   nbd->sock = NULL;
>> - spin_unlock_irq(>sock_lock);
>> -
>> + mutex_unlock(>sock_lock);
>>   del_timer(>timeout_timer);
>>  }
>>
>>  static void nbd_xmit_timeout(unsigned long arg)
>>  {
>>   struct nbd_device *nbd = (struct nbd_device *)arg;
>> - unsigned long flags;
>>
>>   if (list_empty(>queue_head))
>>   return;
>>
>> - spin_lock_irqsave(>sock_lock, flags);
>> -
>> - nbd->timedout = true;
>> -
>> - if (nbd->sock)
>> - kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> -
>> - spin_unlock_irqrestore(>sock_lock, flags);
>> + atomic_inc(>timedout);
>> + wake_up(>waiting_wq);
>>
>>   dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
>> connection\n");
>>  }
>> @@ -579,7 +570,27 @@ static int nbd_thread_send(void *data)
>>   /* wait for something to do */
>>   wait_event_interruptible(nbd->waiting_wq,
>>kthread_should_stop() ||
>> -  !list_empty(>waiting_queue));
>> +  !list_empty(>waiting_queue) ||
>> +  atomic_read(>timedout));
>> +
>> + if (atomic_read(>timedout)) {
>> + mutex_lock(>sock_lock);
>> + if (nbd->sock) {
>> + struct request sreq;
>> +
>> + blk_rq_init(NULL, );
>> + sreq.cmd_type = REQ_TYPE_DRV_PRIV;
>> + mutex_lock(>tx_lock);
>> + nbd->disconnect = true;
>> + nbd_send_req(nbd, );
>> + mutex_unlock(>tx_lock);
>> + dev_err(disk_to_dev(nbd->disk),
>> + "Device Timeout occured.Shutting down"
>> + " socket.");
>> + }
>> + mutex_unlock(>sock_lock);
>> + sock_shutdown(nbd);
>
> Why are you trying to send something on a connection that timed out
> (nbd_send_req())? And afterwards you execute a socket shutdown so in most
> timeout cases this won't reach the server and we risk a blocking send on
> a timedout connection.

Ok. I get it. But shouldn't the server side also close it's socket as
well? I don't
think the timeout value is propagated to server or like server can
"ping" to check
if client is there right?

I agree on nbd_send_req in timedout, it shouldn't be there, just a
sock_shutdown should
do. Can you confirm if I'm right about nbd-server side as 

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  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 
>> ---
>>  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(>tx_lock);
>>   nbd_clear_que(nbd);
>> + /*
>> +  * Wait for any users currently using
>> +  * this block device.
>> +  */
>> + mutex_unlock(>tx_lock);
>> + pr_info("Waiting for users to release device %s ...\n",
>> + bdev->bd_disk->disk_name);
>> + wait_on_bit(>bflags, NBD_BFLAG_INUSE_BIT, 
>> TASK_INTERRUPTIBLE);
>> + mutex_lock(>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, _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, _dev->bflags);
>> + smp_mb();
>> + wake_up_bit(_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. 

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  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 
>> ---
>>  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(>tx_lock);
>>   nbd_clear_que(nbd);
>> + /*
>> +  * Wait for any users currently using
>> +  * this block device.
>> +  */
>> + mutex_unlock(>tx_lock);
>> + pr_info("Waiting for users to release device %s ...\n",
>> + bdev->bd_disk->disk_name);
>> + wait_on_bit(>bflags, NBD_BFLAG_INUSE_BIT, 
>> TASK_INTERRUPTIBLE);
>> + mutex_lock(>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, _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, _dev->bflags);
>> + smp_mb();
>> + wake_up_bit(_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.   | 

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann <m...@pengutronix.de> wrote:
> Hi,
>
> On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote:
>> Hi Markus,
>>
>>
>> On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann <m...@pengutronix.de> wrote:
>> > spinlocked ranges should be small and not contain calls into huge
>> > subfunctions. Fix my mistake and just get the pointer to the socket
>> > instead of doing everything with spinlock held.
>> >
>> > Reported-by: Mikulas Patocka <miku...@twibright.com>
>> > Signed-off-by: Markus Pargmann <m...@pengutronix.de>
>> > ---
>> >  drivers/block/nbd.c | 18 ++
>> >  1 file changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 0b892eed06a0..157bf3da876e 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> > struct request *req)
>> >   */
>> >  static void sock_shutdown(struct nbd_device *nbd)
>> >  {
>> > +   struct socket *sock;
>> > +
>> > spin_lock_irq(>sock_lock);
>> > +   sock = nbd->sock;
>> > +   nbd->sock = NULL;
>> > +   spin_unlock_irq(>sock_lock);
>> >
>> > -   if (!nbd->sock) {
>> > -   spin_unlock_irq(>sock_lock);
>> > +   if (!sock)
>> > return;
>> > -   }
>> > +
>> > +   del_timer(>timeout_timer);
>> >
>> > dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> > -   sockfd_put(nbd->sock);
>> > -   nbd->sock = NULL;
>> > -   spin_unlock_irq(>sock_lock);
>> > +   kernel_sock_shutdown(sock, SHUT_RDWR);
>> > +   sockfd_put(sock);
>> >
>> > -   del_timer(>timeout_timer);
>> >  }
>> >
>> >  static void nbd_xmit_timeout(unsigned long arg)
>>
>> I was concerned about nbd_xmit_timeout as well. There's also a call to
>> kernel_sock_shutdown,
>> while holding the spin_lock in the timeout. The above is ok for
>> sock_shutdown but some kind of change
>> is also required in nbd_xmit_timeout as well. My patch addressed both these.
>
> Oh I see thanks. Seems there is some duplicate code in
> nbd_xmit_timeout and sock_shutdown. I think nbd_xmit_timeout could
> perhaps be simplified?
>
> static void nbd_xmit_timeout(unsigned long arg)
> {
> struct nbd_device *nbd = (struct nbd_device *)arg;
>
> if (list_empty(>queue_head))
> return;
>
> nbd->timedout = true;
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>
> sock_shutdown(nbd);

Even then, the timeout is non-process context so there will still be a
might_sleep warning.
So sock_shutdown itself can be simplified, as you posted in earlier patch,
but I don't think sock_shutdown should be called from non-process context[?].

> }
>
> Thanks,
>
> Markus
>
> --
> 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- |



-- 
---P.K.S

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

On Thu, May 12, 2016 at 6:13 PM, Markus Pargmann  wrote:
> Hi,
>
> On Thursday 12 May 2016 16:42:31 Pranay Srivastava wrote:
>> Hi Markus,
>>
>>
>> On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann  wrote:
>> > spinlocked ranges should be small and not contain calls into huge
>> > subfunctions. Fix my mistake and just get the pointer to the socket
>> > instead of doing everything with spinlock held.
>> >
>> > Reported-by: Mikulas Patocka 
>> > Signed-off-by: Markus Pargmann 
>> > ---
>> >  drivers/block/nbd.c | 18 ++
>> >  1 file changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 0b892eed06a0..157bf3da876e 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, 
>> > struct request *req)
>> >   */
>> >  static void sock_shutdown(struct nbd_device *nbd)
>> >  {
>> > +   struct socket *sock;
>> > +
>> > spin_lock_irq(>sock_lock);
>> > +   sock = nbd->sock;
>> > +   nbd->sock = NULL;
>> > +   spin_unlock_irq(>sock_lock);
>> >
>> > -   if (!nbd->sock) {
>> > -   spin_unlock_irq(>sock_lock);
>> > +   if (!sock)
>> > return;
>> > -   }
>> > +
>> > +   del_timer(>timeout_timer);
>> >
>> > dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
>> > -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
>> > -   sockfd_put(nbd->sock);
>> > -   nbd->sock = NULL;
>> > -   spin_unlock_irq(>sock_lock);
>> > +   kernel_sock_shutdown(sock, SHUT_RDWR);
>> > +   sockfd_put(sock);
>> >
>> > -   del_timer(>timeout_timer);
>> >  }
>> >
>> >  static void nbd_xmit_timeout(unsigned long arg)
>>
>> I was concerned about nbd_xmit_timeout as well. There's also a call to
>> kernel_sock_shutdown,
>> while holding the spin_lock in the timeout. The above is ok for
>> sock_shutdown but some kind of change
>> is also required in nbd_xmit_timeout as well. My patch addressed both these.
>
> Oh I see thanks. Seems there is some duplicate code in
> nbd_xmit_timeout and sock_shutdown. I think nbd_xmit_timeout could
> perhaps be simplified?
>
> static void nbd_xmit_timeout(unsigned long arg)
> {
> struct nbd_device *nbd = (struct nbd_device *)arg;
>
> if (list_empty(>queue_head))
> return;
>
> nbd->timedout = true;
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>
> sock_shutdown(nbd);

Even then, the timeout is non-process context so there will still be a
might_sleep warning.
So sock_shutdown itself can be simplified, as you posted in earlier patch,
but I don't think sock_shutdown should be called from non-process context[?].

> }
>
> Thanks,
>
> Markus
>
> --
> 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- |



-- 
---P.K.S

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

Hi Markus,


On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann  wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
> ---
>  drivers/block/nbd.c | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0b892eed06a0..157bf3da876e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> +   struct socket *sock;
> +
> spin_lock_irq(>sock_lock);
> +   sock = nbd->sock;
> +   nbd->sock = NULL;
> +   spin_unlock_irq(>sock_lock);
>
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> +   if (!sock)
> return;
> -   }
> +
> +   del_timer(>timeout_timer);
>
> dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> -   nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>
> -   del_timer(>timeout_timer);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)

I was concerned about nbd_xmit_timeout as well. There's also a call to
kernel_sock_shutdown,
while holding the spin_lock in the timeout. The above is ok for
sock_shutdown but some kind of change
is also required in nbd_xmit_timeout as well. My patch addressed both these.

Can you have a look at that again.


> --
> 2.8.0.rc3
>



-- 
---P.K.S

Re: [PATCH] nbd: Move socket shutdown out of spinlock

[Pranay Srivastava]

Hi Markus,


On Thu, May 12, 2016 at 3:13 PM, Markus Pargmann  wrote:
> spinlocked ranges should be small and not contain calls into huge
> subfunctions. Fix my mistake and just get the pointer to the socket
> instead of doing everything with spinlock held.
>
> Reported-by: Mikulas Patocka 
> Signed-off-by: Markus Pargmann 
> ---
>  drivers/block/nbd.c | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0b892eed06a0..157bf3da876e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -173,20 +173,22 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> +   struct socket *sock;
> +
> spin_lock_irq(>sock_lock);
> +   sock = nbd->sock;
> +   nbd->sock = NULL;
> +   spin_unlock_irq(>sock_lock);
>
> -   if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> +   if (!sock)
> return;
> -   }
> +
> +   del_timer(>timeout_timer);
>
> dev_warn(disk_to_dev(nbd->disk), "shutting down socket\n");
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -   sockfd_put(nbd->sock);
> -   nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> +   kernel_sock_shutdown(sock, SHUT_RDWR);
> +   sockfd_put(sock);
>
> -   del_timer(>timeout_timer);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)

I was concerned about nbd_xmit_timeout as well. There's also a call to
kernel_sock_shutdown,
while holding the spin_lock in the timeout. The above is ok for
sock_shutdown but some kind of change
is also required in nbd_xmit_timeout as well. My patch addressed both these.

Can you have a look at that again.


> --
> 2.8.0.rc3
>



-- 
---P.K.S

Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Pranay Srivastava]

Greg,

Resending as I missed the cc list earlier.

On Wed, May 11, 2016 at 2:03 PM, Greg KH  wrote:
> On Wed, May 11, 2016 at 11:18:30AM +0300, Pranay Kr. Srivastava wrote:
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm not the maintainer of these files or driver (which is why I don't

The series contained some checkpatch changes so I had included you as well.

> know why you are sending them to me), but I know I do not accept patches
> without any changelog text at all in them, as that's just lazy.

That should be per patch or can appear in a cover letter for the patches?

Actually I've made more patches in this series after I had sent the
earlier ones,
but the earlier ones are not changed at all. It's only the addition of
newer patches
to the series. So I'm not sure how exactly would you like these to be.

>
> But, the nbd.c maintainer might accept them, you might get lucky!
>

Thanks again for the reply. Just in case I don't get any reply from
the maintainer,
which I've not as of today, do I consider these patches as dump or
should I resend
these to somebody else. I would really appreciate if you can answer that.

I still have one more patch to send over these which is an enhancement
rather than
a fix but I've not yet got any review of the patches.


> good luck,
>
> greg k-h


--
---P.K.S


-- 
---P.K.S

Fwd: [PATCH v4 02/18] nbd: fix checkpatch trailing space warning.

[Pranay Srivastava]

Greg,

Resending as I missed the cc list earlier.

On Wed, May 11, 2016 at 2:03 PM, Greg KH  wrote:
> On Wed, May 11, 2016 at 11:18:30AM +0300, Pranay Kr. Srivastava wrote:
>> Signed-off-by: Pranay Kr. Srivastava 
>> ---
>>  drivers/block/nbd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm not the maintainer of these files or driver (which is why I don't

The series contained some checkpatch changes so I had included you as well.

> know why you are sending them to me), but I know I do not accept patches
> without any changelog text at all in them, as that's just lazy.

That should be per patch or can appear in a cover letter for the patches?

Actually I've made more patches in this series after I had sent the
earlier ones,
but the earlier ones are not changed at all. It's only the addition of
newer patches
to the series. So I'm not sure how exactly would you like these to be.

>
> But, the nbd.c maintainer might accept them, you might get lucky!
>

Thanks again for the reply. Just in case I don't get any reply from
the maintainer,
which I've not as of today, do I consider these patches as dump or
should I resend
these to somebody else. I would really appreciate if you can answer that.

I still have one more patch to send over these which is an enhancement
rather than
a fix but I've not yet got any review of the patches.


> good luck,
>
> greg k-h


--
---P.K.S


-- 
---P.K.S

Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Tue, May 3, 2016 at 5:45 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Mon, May 02, 2016 at 08:58:34AM +0530, Pranay Srivastava wrote:
>> Hi,
>>
>> Can the following patch be reviewed? I'm working on some more changes
>> on top of this change,
>> so it'll be really helpful if someone can review this patch and let me
>> know of shortcomings/issues
>> with this.
>>
>>
>> On Sat, Apr 30, 2016 at 11:49 AM, Pranay Kr. Srivastava
>> <pran...@gmail.com> wrote:
>> > This patch fixes the warning generated when a timeout occurs
>> > on the request and socket is closed from a non-sleep context
>> > by
>> >
>> > 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > 2. Make sock lock to be a mutex instead of a spin lock, since
>> >nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > 3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.
>
> Why are you doing three things in one patch?
>
>> > ---
>> >  drivers/block/nbd.c | 85 
>> > +++--
>> >  1 file changed, 50 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 31e73a7..a52cc16 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -3,7 +3,7 @@
>> >   *
>> >   * Note that you can not swap over this thing, yet. Seems to work but
>> >   * deadlocks sometimes - you can not swap over TCP in general.
>> > - *
>> > + *
>
> What did you change here?
>
>> >   * Copyright 1997-2000, 2008 Pavel Machek <pa...@ucw.cz>
>> >   * Parts copyright 2001 Steven Whitehouse <st...@chygwyn.com>
>> >   *
>> > @@ -35,14 +35,14 @@
>> >  #include 
>> >  #include 
>> >
>> > -#include 
>> > +#include 
>
> Why change this?
>
>> >  #include 
>> >
>> >  #include 
>> >
>> >  struct nbd_device {
>> > u32 flags;
>> > -   struct socket * sock;   /* If == NULL, device is not ready, yet */
>> > +   struct socket *sock;/* If == NULL, device is not ready, yet */
>
> You are mixing "code cleanup" changes with "change the logic" changes,
> please break this up into a series of patches, doing the code cleanup
> _last_ as you want to fix bugs first.
>

Ok. Will do that.

> thanks,
>
> greg k-h

Thanks alot Greg for review.

-- 
---P.K.S

Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

On Tue, May 3, 2016 at 5:45 AM, Greg KH  wrote:
> On Mon, May 02, 2016 at 08:58:34AM +0530, Pranay Srivastava wrote:
>> Hi,
>>
>> Can the following patch be reviewed? I'm working on some more changes
>> on top of this change,
>> so it'll be really helpful if someone can review this patch and let me
>> know of shortcomings/issues
>> with this.
>>
>>
>> On Sat, Apr 30, 2016 at 11:49 AM, Pranay Kr. Srivastava
>>  wrote:
>> > This patch fixes the warning generated when a timeout occurs
>> > on the request and socket is closed from a non-sleep context
>> > by
>> >
>> > 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > 2. Make sock lock to be a mutex instead of a spin lock, since
>> >nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > 3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.
>
> Why are you doing three things in one patch?
>
>> > ---
>> >  drivers/block/nbd.c | 85 
>> > +++--
>> >  1 file changed, 50 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 31e73a7..a52cc16 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -3,7 +3,7 @@
>> >   *
>> >   * Note that you can not swap over this thing, yet. Seems to work but
>> >   * deadlocks sometimes - you can not swap over TCP in general.
>> > - *
>> > + *
>
> What did you change here?
>
>> >   * Copyright 1997-2000, 2008 Pavel Machek 
>> >   * Parts copyright 2001 Steven Whitehouse 
>> >   *
>> > @@ -35,14 +35,14 @@
>> >  #include 
>> >  #include 
>> >
>> > -#include 
>> > +#include 
>
> Why change this?
>
>> >  #include 
>> >
>> >  #include 
>> >
>> >  struct nbd_device {
>> > u32 flags;
>> > -   struct socket * sock;   /* If == NULL, device is not ready, yet */
>> > +   struct socket *sock;/* If == NULL, device is not ready, yet */
>
> You are mixing "code cleanup" changes with "change the logic" changes,
> please break this up into a series of patches, doing the code cleanup
> _last_ as you want to fix bugs first.
>

Ok. Will do that.

> thanks,
>
> greg k-h

Thanks alot Greg for review.

-- 
---P.K.S

Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

Hi,

Can the following patch be reviewed? I'm working on some more changes
on top of this change,
so it'll be really helpful if someone can review this patch and let me
know of shortcomings/issues
with this.


On Sat, Apr 30, 2016 at 11:49 AM, Pranay Kr. Srivastava
 wrote:
> This patch fixes the warning generated when a timeout occurs
> on the request and socket is closed from a non-sleep context
> by
>
> 1. Moving the socket closing on a timeout to nbd_thread_send
>
> 2. Make sock lock to be a mutex instead of a spin lock, since
>nbd_xmit_timeout doesn't need to hold it anymore.
>
> 3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.
> ---
>  drivers/block/nbd.c | 85 
> +++--
>  1 file changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..a52cc16 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -3,7 +3,7 @@
>   *
>   * Note that you can not swap over this thing, yet. Seems to work but
>   * deadlocks sometimes - you can not swap over TCP in general.
> - *
> + *
>   * Copyright 1997-2000, 2008 Pavel Machek 
>   * Parts copyright 2001 Steven Whitehouse 
>   *
> @@ -35,14 +35,14 @@
>  #include 
>  #include 
>
> -#include 
> +#include 
>  #include 
>
>  #include 
>
>  struct nbd_device {
> u32 flags;
> -   struct socket * sock;   /* If == NULL, device is not ready, yet */
> +   struct socket *sock;/* If == NULL, device is not ready, yet */
> int magic;
>
> spinlock_t queue_lock;
> @@ -57,12 +57,12 @@ struct nbd_device {
> int blksize;
> loff_t bytesize;
> int xmit_timeout;
> -   bool timedout;
> +   atomic_t timedout;
> bool disconnect; /* a disconnect has been requested by user */
>
> struct timer_list timeout_timer;
> /* protects initialization and shutdown of the socket */
> -   spinlock_t sock_lock;
> +   struct mutex sock_lock;
> struct task_struct *task_recv;
> struct task_struct *task_send;
>
> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> +   mutex_lock(>sock_lock);
> if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> +   mutex_unlock(>sock_lock);
> return;
> }
>
> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
> kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> sockfd_put(nbd->sock);
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> -
> +   mutex_unlock(>sock_lock);
> del_timer(>timeout_timer);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
>
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> -   nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> +   atomic_inc(>timedout);
> +   wake_up(>waiting_wq);
>
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
> @@ -266,6 +257,7 @@ static inline int sock_send_bvec(struct nbd_device *nbd, 
> struct bio_vec *bvec,
>  {
> int result;
> void *kaddr = kmap(bvec->bv_page);
> +
> result = sock_xmit(nbd, 1, kaddr + bvec->bv_offset,
>bvec->bv_len, flags);
> kunmap(bvec->bv_page);
> @@ -278,6 +270,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct 
> request *req)
> int result, flags;
> struct nbd_request request;
> unsigned long size = blk_rq_bytes(req);
> +
> u32 type;
>
> if (req->cmd_type == REQ_TYPE_DRV_PRIV)
> @@ -363,6 +356,7 @@ static inline int sock_recv_bvec(struct nbd_device *nbd, 
> struct bio_vec *bvec)
>  {
> int result;
> void *kaddr = kmap(bvec->bv_page);
> +
> result = sock_xmit(nbd, 0, kaddr + bvec->bv_offset, bvec->bv_len,
> MSG_WAITALL);
> kunmap(bvec->bv_page);
> @@ -579,7 +573,27 @@ static int nbd_thread_send(void *data)
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
>  kthread_should_stop() ||
> -!list_empty(>waiting_queue));
> +!list_empty(>waiting_queue) ||
> +atomic_read(>timedout));
> +
> +   if (atomic_read(>timedout)) {
> +   mutex_lock(>sock_lock);
> +   if (nbd->sock) {

Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

[Pranay Srivastava]

Hi,

Can the following patch be reviewed? I'm working on some more changes
on top of this change,
so it'll be really helpful if someone can review this patch and let me
know of shortcomings/issues
with this.


On Sat, Apr 30, 2016 at 11:49 AM, Pranay Kr. Srivastava
 wrote:
> This patch fixes the warning generated when a timeout occurs
> on the request and socket is closed from a non-sleep context
> by
>
> 1. Moving the socket closing on a timeout to nbd_thread_send
>
> 2. Make sock lock to be a mutex instead of a spin lock, since
>nbd_xmit_timeout doesn't need to hold it anymore.
>
> 3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.
> ---
>  drivers/block/nbd.c | 85 
> +++--
>  1 file changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 31e73a7..a52cc16 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -3,7 +3,7 @@
>   *
>   * Note that you can not swap over this thing, yet. Seems to work but
>   * deadlocks sometimes - you can not swap over TCP in general.
> - *
> + *
>   * Copyright 1997-2000, 2008 Pavel Machek 
>   * Parts copyright 2001 Steven Whitehouse 
>   *
> @@ -35,14 +35,14 @@
>  #include 
>  #include 
>
> -#include 
> +#include 
>  #include 
>
>  #include 
>
>  struct nbd_device {
> u32 flags;
> -   struct socket * sock;   /* If == NULL, device is not ready, yet */
> +   struct socket *sock;/* If == NULL, device is not ready, yet */
> int magic;
>
> spinlock_t queue_lock;
> @@ -57,12 +57,12 @@ struct nbd_device {
> int blksize;
> loff_t bytesize;
> int xmit_timeout;
> -   bool timedout;
> +   atomic_t timedout;
> bool disconnect; /* a disconnect has been requested by user */
>
> struct timer_list timeout_timer;
> /* protects initialization and shutdown of the socket */
> -   spinlock_t sock_lock;
> +   struct mutex sock_lock;
> struct task_struct *task_recv;
> struct task_struct *task_send;
>
> @@ -172,10 +172,9 @@ static void nbd_end_request(struct nbd_device *nbd, 
> struct request *req)
>   */
>  static void sock_shutdown(struct nbd_device *nbd)
>  {
> -   spin_lock_irq(>sock_lock);
> -
> +   mutex_lock(>sock_lock);
> if (!nbd->sock) {
> -   spin_unlock_irq(>sock_lock);
> +   mutex_unlock(>sock_lock);
> return;
> }
>
> @@ -183,27 +182,19 @@ static void sock_shutdown(struct nbd_device *nbd)
> kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> sockfd_put(nbd->sock);
> nbd->sock = NULL;
> -   spin_unlock_irq(>sock_lock);
> -
> +   mutex_unlock(>sock_lock);
> del_timer(>timeout_timer);
>  }
>
>  static void nbd_xmit_timeout(unsigned long arg)
>  {
> struct nbd_device *nbd = (struct nbd_device *)arg;
> -   unsigned long flags;
>
> if (list_empty(>queue_head))
> return;
>
> -   spin_lock_irqsave(>sock_lock, flags);
> -
> -   nbd->timedout = true;
> -
> -   if (nbd->sock)
> -   kernel_sock_shutdown(nbd->sock, SHUT_RDWR);
> -
> -   spin_unlock_irqrestore(>sock_lock, flags);
> +   atomic_inc(>timedout);
> +   wake_up(>waiting_wq);
>
> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down 
> connection\n");
>  }
> @@ -266,6 +257,7 @@ static inline int sock_send_bvec(struct nbd_device *nbd, 
> struct bio_vec *bvec,
>  {
> int result;
> void *kaddr = kmap(bvec->bv_page);
> +
> result = sock_xmit(nbd, 1, kaddr + bvec->bv_offset,
>bvec->bv_len, flags);
> kunmap(bvec->bv_page);
> @@ -278,6 +270,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct 
> request *req)
> int result, flags;
> struct nbd_request request;
> unsigned long size = blk_rq_bytes(req);
> +
> u32 type;
>
> if (req->cmd_type == REQ_TYPE_DRV_PRIV)
> @@ -363,6 +356,7 @@ static inline int sock_recv_bvec(struct nbd_device *nbd, 
> struct bio_vec *bvec)
>  {
> int result;
> void *kaddr = kmap(bvec->bv_page);
> +
> result = sock_xmit(nbd, 0, kaddr + bvec->bv_offset, bvec->bv_len,
> MSG_WAITALL);
> kunmap(bvec->bv_page);
> @@ -579,7 +573,27 @@ static int nbd_thread_send(void *data)
> /* wait for something to do */
> wait_event_interruptible(nbd->waiting_wq,
>  kthread_should_stop() ||
> -!list_empty(>waiting_queue));
> +!list_empty(>waiting_queue) ||
> +atomic_read(>timedout));
> +
> +   if (atomic_read(>timedout)) {
> +   mutex_lock(>sock_lock);
> +   if (nbd->sock) {
> +   struct request