Re: [Nbd] [PATCH] NBD: replace kill_bdev() with __invalidate_device()

[Markus Pargmann <mpa@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

Hi,

On Thursday 28 April 2016 18:27:34 Wouter Verhelst wrote:
> On Thu, Apr 28, 2016 at 11:00:20AM +0200, Markus Pargmann wrote:
> > Hi,
> > 
> > On Saturday 23 April 2016 07:47:21 Ratna Manoj wrote:
> > > Thanks for the review. 
> > > 
> > > Atleast for ext4 this crash happens on a sys_umount() call, timing of
> > > which is not in control of block driver. Block driver cannot force the
> > > filesystems to be unmounted, and the file system does not expect 
> > > buffers to get unmapped under it.
> > 
> > Yes the block driver can't force a clean umount.
> > 
> > > 
> > > Ext4 can be fixed with the this patch:
> > > http://www.spinics.net/lists/linux-ext4/msg51112.html 
> > > It did not make to the kernel. It checks the state of the buffer head
> > > before committing.
> > > 
> > > When we consider diskett/CD as user space thread that called NBD_DO_IT,
> > > this problem is analogous to changing disk with another or the same
> > > disk suddenly when the file system is still mounted. 
> > > 
> > > If we completely kill the block device we would loss some writes when
> > > same thread is reconnected.
> > 
> > I am not so sure about your exact use-case here.
> > 
> > If the NBD_DO_IT thread returns I am considering the connection and
> > block device as dead and disconnected. Securing any data afterwards with
> > a new connection is potentially dangerous as it may be a different
> > server.
> 
> Yes, from the kernel POV you're right.
> 
> However, at some point I agreed with Paul (your predecessor) that when
> this happens due to an error condition (as opposed to it being due to an
> explicit disconnect), the kernel would block all reads from or writes to
> the device, and the client may try to reconnect *from the same
> PID* (i.e., it may not fork()). If that succeeds, the next NBD_DO_IT is
> assumed to be connected to the same server; if instead the process
> exits, then the block device is assumed to be dead, will be reset, and
> all pending reads or writes would error.
> 
> In principle, this allows for a proper reconnect from userspace if it
> can be done. However, I'm not sure whether this ever worked well or
> whether it was documented, so it's probably fine if you think it should
> be replaced with something else.

At least I was not aware of this possibility. As far as I know the
previous code even had issues with the signals used to kill on timeouts
which also killed the userspace program sometimes.

Currently I can't see a code path that supports reconnects. But I may
have removed that accidently in the past.

> 
> (obviously, userspace reconnecting the device to a different device is
> wrong and should not be done, but that's a case of "if you break it, you
> get to keep both pieces)
> 
> At any rate, I think it makes sense for userspace to be given a chance
> to *attempt* to reconnect a device when the connection drops
> unexpectedly.

Perhaps it would be better to setup the kernel driver explicitly for
that. Perhaps some flag to let the kernel driver know that the client
would like to keep the block device open? In that case the client could
excplicitly use NBD_CLEAR_SOCK to cleanup everything.

Or perhaps a completely new ioctl that can transmit back some more
information about what failures were seen and whether the blockdevice
was closed or not?

Best Regards,

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]