From: Markus Pargmann <mpa@pengutronix.de>
To: Ratna Manoj <manoj.br@gmail.com>
Cc: pbonzini@redhat.com, jack@suse.cz, grao@portworx.com, jv@portworx.com,
        nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] NBD: replace kill_bdev() with __invalidate_device()
Date: Wed, 20 Apr 2016 13:06:34 +0200
Message-ID: <11986900.ymBh9azTyz@adelgunde>
User-Agent: KMail/4.14.1 (Linux/4.4.0-0.bpo.1-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <56F34412.1050707@gmail.com>
References: <56E711D1.9090708@gmail.com> <2358965.KUbuAa1sts@adelgunde> <56F34412.1050707@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart66426185.IxdhjnmDck"; micalg="pgp-sha256"; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4921
Lines: 154


--nextPart66426185.IxdhjnmDck
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="us-ascii"

Hi,

On Thursday 24 March 2016 07:04:10 Ratna Manoj wrote:
> From: Ratna Manoj Bolla <manoj.br@gmail.com>
>=20
> When a filesystem is mounted on a nbd device and on a disconnect, bec=
ause=20
> of kill_bdev(), and resetting bdev size to zero, buffer_head mappings=
 are=20
> getting destroyed under mounted filesystem.
>=20
> After a bdev size reset(i.e bdev->bd_inode->i_size =3D 0) on a discon=
nect,
> followed by a sys_umount(),
>         generic_shutdown_super()->...
>         ->__sync_blockdev()->...
>         -> blkdev_writepages()->...
>         ->do_invalidatepage()->...
>         -> discard_buffer()   is discarding superblock buffer_head as=
sumed
> to be in mapped state by ext4_commit_super().
>=20
>=20
>=20
> Signed-off-by: Ratna Manoj Bolla <manoj.br@gmail.com>
> ---
> This script reproduces both the kernel panic scenarios:
> =20
> $ qemu-img create -f qcow2 f.img 1G
> $ mkfs.ext4 f.img
> $ qemu-nbd -c /dev/nbd0 f.img
> $ mount /dev/nbd0 dir
> $ killall -KILL qemu-nbd
> $ sleep 1
> $ ls dir
> $ umount dir
>=20
> Bug reports:
> http://www.kernelhub.org/?p=3D2&msg=3D361407
> https://www.mail-archive.com/nbd-general@lists.sourceforge.net/msg023=
88.html

Thanks, please CC nbd-general@lists.sourceforge.net,
linux-kernel@vger.kernel.org as well.

So this patch simply does not cleanup the blockdevice to avoid any
errors on the filesystem side. The userspace thread that called
NBD_DO_IT will exit immediately before the filesystem decided to releas=
e
the blockdevice. The nbd driver assumes that the shutdown was done and
accepts new clients setting up sockets and so on. Couldn't this lead to=

a lot of problems?

Currently NBD_DO_IT returns when it is save to use the NBD device again=
.
This patch changes this as the blockdevice may still be in use when
NBD_DO_IT returns. I think it would be better to delay NBD_DO_IT until
everything is cleaned up and all filesystems are closed.

Best Regards,

Markus

>=20
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index f6b51d7..6e77b3a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -119,7 +119,8 @@ static const char *nbdcmd_to_ascii(int cmd)
> =20
>  static int nbd_size_clear(struct nbd_device *nbd, struct block_devic=
e *bdev)
>  {
> -=09bdev->bd_inode->i_size =3D 0;
> +=09if (bdev->bd_openers <=3D 1)
> +=09=09bdev->bd_inode->i_size =3D 0;
>  =09set_capacity(nbd->disk, 0);
>  =09kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
> =20
> @@ -678,6 +679,9 @@ static void nbd_reset(struct nbd_device *nbd)
> =20
>  static void nbd_bdev_reset(struct block_device *bdev)
>  {
> +=09if (bdev->bd_openers > 1)
> +=09=09return;
> +
>  =09set_device_ro(bdev, false);
>  =09bdev->bd_inode->i_size =3D 0;
>  =09if (max_part > 0) {
> @@ -735,7 +739,7 @@ static int __nbd_ioctl(struct block_device *bdev,=
 struct nbd_device *nbd,
>  =09=09nbd_clear_que(nbd);
>  =09=09BUG_ON(!list_empty(&nbd->queue_head));
>  =09=09BUG_ON(!list_empty(&nbd->waiting_queue));
> -=09=09kill_bdev(bdev);
> +=09=09__invalidate_device(bdev, true);
>  =09=09return 0;
> =20
>  =09case NBD_SET_SOCK: {
> @@ -809,7 +813,7 @@ static int __nbd_ioctl(struct block_device *bdev,=
 struct nbd_device *nbd,
> =20
>  =09=09sock_shutdown(nbd);
>  =09=09nbd_clear_que(nbd);
> -=09=09kill_bdev(bdev);
> +=09=09__invalidate_device(bdev, true);
>  =09=09nbd_bdev_reset(bdev);
> =20
>  =09=09if (nbd->disconnect) /* user requested, ignore socket errors *=
/
>=20
>=20
>=20

=2D-=20
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-555=
5 |

--nextPart66426185.IxdhjnmDck
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJXF2K6AAoJEEpcgKtcEGQQAPAP/AmqvkSStqUu2dQRc/xp9oXX
8FCqF7Cv2/fQ+44iltHD+zu2/cWBZ4xRIF1LisqeNmW4MfwqNmgydE3Vd7gQ12Vq
G4DFWmbhNpL+RoDmrwZ2k8nI7cVdvFwUbpfRqKT0XQew9B94kmWhirYCg5R9H7EB
dEROuHMwdmDLqhUOzj+hAz3CdEHZscp7lMkZ3lnXRYliQg+QmCCviOrInXK5PSND
cHkzIlschzz5huQu4wUaQud63wpsUvH18u4XhG3xOWZYXthBY4YazjgHeR2Alq81
wUOhJHL1IKp/7bx6hxklcimsdftkA1ud0G9JFu4U8JWb1F+tZLcOMCEqL8E7iWmO
V/rATtdrDpSEjDXOCyp3W0vHNkJxs6P572QPhsF7GhdcJUFFGcOFI1Ejj1M4QvKC
1xco0CF8Otl+S03J5s79rlwwZXH3gdcaGfMx/hixezhPH72IPrvcJsiUET0ZnejC
rbA0juUAgjM3YoTUHhIxGXLe9gVg9D3CWDUsqvQtoAGvnanS40FVT6SRw7lYcBQX
JbYYZOQM6ysRj94uaBGQSAK7/qCbqNB6mJ7GPrHRgdAivq4LFbMlv2QZveu4dzLM
1GDTCaRc1p5eEceyyYfXs+IocooX015JRRRSGi/ThIq4phqrT72NWpmRDjtyndio
4vTWYFLsKrY08QgjnVhK
=JOMA
-----END PGP SIGNATURE-----

--nextPart66426185.IxdhjnmDck--