From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754228AbcEOM4B (ORCPT ); Sun, 15 May 2016 08:56:01 -0400 Received: from barbershop.grep.be ([89.106.240.122]:51271 "EHLO barbershop.grep.be" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754069AbcEOM4A (ORCPT ); Sun, 15 May 2016 08:56:00 -0400 Date: Sun, 15 May 2016 14:55:39 +0200 From: Wouter Verhelst To: Markus Pargmann Cc: Ratna Manoj , nbd-general@lists.sourceforge.net, Vinod Jayaraman , jack@suse.cz, linux-kernel@vger.kernel.org, Gou Rao , pbonzini@redhat.com Subject: Re: [Nbd] [PATCH] NBD: replace kill_bdev() with __invalidate_device() Message-ID: <20160515125539.GA14784@grep.be> References: <56E711D1.9090708@gmail.com> <1809196.i4ZeAM11iZ@adelgunde> <20160428162734.GA3082@grep.be> <1733802.dNuTe5BCE4@adelgunde> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1733802.dNuTe5BCE4@adelgunde> X-Speed: Gates' Law: Every 18 months, the speed of software halves. Organization: none User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Markus, On Thu, May 12, 2016 at 11:53:01AM +0200, Markus Pargmann wrote: > On Thursday 28 April 2016 18:27:34 Wouter Verhelst wrote: > > 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. Right. Like I said, I'm not sure if it ever worked well. The user space client has a -persist option that tries to implement it, but I've been getting some bug reports from people who've tried it (although that may have been my fault rather than the kernel's). > > (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. I'm not sure what you mean by this. Can you clarify? > 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? The intent was that ioctl(NBD_DO_IT) would return an error when the disconnect was not requested, and would return 0 when the connection dropped due to userspace doing ioctl(NBD_DISCONNECT), since dropping the connection when userspace explicitly asks for it is not an error. drivers/block/nbd.c contains the following: static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, unsigned int cmd, unsigned long arg) { [...] case NBD_DO_IT: { [...] if (nbd->disconnect) /* user requested, ignore socket errors */ return 0; return error; } [...] so the signalling part of it is at least still there. Whether it works, I haven't tested. -- < 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