subject:"\[PATCH v4 3\/5\]nbd\: make nbd device wait for its users"

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

2016-07-13 Thread 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;
>> >> @@ -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

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

2016-07-13 Thread 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;
>> >> @@ -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,

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

2016-07-13 Thread Markus Pargmann

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?

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

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

2016-07-13 Thread Markus Pargmann

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?

> 
> >
> >>   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: {
> >> @@

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

2016-07-10 Thread 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

2016-07-10 Thread 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 3/5]nbd: make nbd device wait for its users

2016-07-10 Thread Markus Pargmann

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?

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

>   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 = nbd_thread_recv(nbd, bdev);
>   nbd_dev_dbg_close(nbd);
>   kthread_stop(thread);
> - sock_shutdown(nbd);
> -
> - mutex_lock(>tx_lock);
> - nbd->task_recv = NULL;
> 
> - nbd_clear_que(nbd);
> - kill_bdev(bdev);
> - nbd_bdev_reset(bdev);
> + sock_shutdown(nbd);
> 
>   if (nbd->disconnect) /* user

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

2016-07-10 Thread Markus Pargmann

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?

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

>   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 = nbd_thread_recv(nbd, bdev);
>   nbd_dev_dbg_close(nbd);
>   kthread_stop(thread);
> - sock_shutdown(nbd);
> -
> - mutex_lock(>tx_lock);
> - nbd->task_recv = NULL;
> 
> - nbd_clear_que(nbd);
> - kill_bdev(bdev);
> - nbd_bdev_reset(bdev);
> + sock_shutdown(nbd);
> 
>   if (nbd->disconnect) /* user requested, ignore

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

2016-06-30 Thread Pranay Kr. Srivastava

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 *);
 
 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;
-
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;
+
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;
else
nbd->sock = sock;
-   spin_unlock(>sock_lock);
 
+   spin_unlock(>sock_lock);
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);
-
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 = nbd_thread_recv(nbd, bdev);
nbd_dev_dbg_close(nbd);
kthread_stop(thread);
-   sock_shutdown(nbd);
-
-   mutex_lock(>tx_lock);
-   nbd->task_recv = NULL;
 
-   nbd_clear_que(nbd);
-   kill_bdev(bdev);
-   nbd_bdev_reset(bdev);
+   sock_shutdown(nbd);
 
if (nbd->disconnect) /* user requested, ignore socket errors */
error = 0;
if (nbd->timedout)
error = -ETIMEDOUT;
 
-   nbd_reset(nbd);
+   mutex_lock(>tx_lock);
+   nbd_clear_que(nbd);
+   nbd->disconnect = true; /* To kill bdev*/
+

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

2016-06-30 Thread Pranay Kr. Srivastava

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 *);
 
 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;
-
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;
+
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;
else
nbd->sock = sock;
-   spin_unlock(>sock_lock);
 
+   spin_unlock(>sock_lock);
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);
-
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 = nbd_thread_recv(nbd, bdev);
nbd_dev_dbg_close(nbd);
kthread_stop(thread);
-   sock_shutdown(nbd);
-
-   mutex_lock(>tx_lock);
-   nbd->task_recv = NULL;
 
-   nbd_clear_que(nbd);
-   kill_bdev(bdev);
-   nbd_bdev_reset(bdev);
+   sock_shutdown(nbd);
 
if (nbd->disconnect) /* user requested, ignore socket errors */
error = 0;
if (nbd->timedout)
error = -ETIMEDOUT;
 
-   nbd_reset(nbd);
+   mutex_lock(>tx_lock);
+   nbd_clear_que(nbd);
+   nbd->disconnect = true; /* To kill bdev*/
+   mutex_unlock(>tx_lock);
+

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

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

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

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

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

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

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

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

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

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

10 matches

Site Navigation

Mail list logo

Footer information