Re: [Nbd] [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: [Nbd] [PATCH 3/3]nbd: make nbd device wait for its users

[Markus Pargmann]

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
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.

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,
> - 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 

Re: [Nbd] [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: [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

--
Attend Shape: An AT Tech Expo July 15-16. Meet us at AT Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

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

[Eric Blake]

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/

> 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



signature.asc
Description: OpenPGP digital signature
--
Attend Shape: An AT Tech Expo July 15-16. Meet us at AT Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general