Feedback-ID: i12284293:Fastmail
Date:   Sat, 18 Mar 2023 01:50:05 +0000
From:   Alyssa Ross <hi@alyssa.is>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
        Martijn Coenen <maco@android.com>, linux-block@vger.kernel.org
Subject: Re: [PATCH v2] loop: LOOP_CONFIGURE: send uevents for partitions
Message-ID: <20230318015005.czydsbqpw6hnej3z@x220>
References: <20230312191031.551204-1-hi@alyssa.is>
 <ZBHo2AXYM0iVkXvO@infradead.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="br25mg72tdjmjgjf"
Content-Disposition: inline
In-Reply-To: <ZBHo2AXYM0iVkXvO@infradead.org>
Precedence: bulk


--br25mg72tdjmjgjf
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 15, 2023 at 08:48:40AM -0700, Christoph Hellwig wrote:
> On Sun, Mar 12, 2023 at 07:10:31PM +0000, Alyssa Ross wrote:
> > +	 * Now that we are done, reread the partitions with uevent
> > +	 * re-enabled if appropriate to let userspace know about the
> > +	 * changes.
> > +	 */
> > +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), !partscan_uevent);
> > +	if (partscan)
> > +		loop_reread_partitions(lo);
> > +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
>
> What worries me here is that you move the partition re-read out of
> the exclusive claim, which is another potentially user visible
> change (and user visible behavior changes are a field of landmines
> in loop as you have noticed).

Makes sense.

> But in the end we only need to suppress the events until Lo_Bound
> is set.  So something like the patch below that reduces the no even
> critical section might do the job?

If you say so!  I had trouble understanding which parts of the function
uevents needed to be suppressed for, so I was trying to move as little
as possible out of that section.

What happens next?  I'm still getting up to speed on the kernel
development process =E2=80=94 will you submit this as a patch with a patch =
body
and a S-o-b?  Or am I supposed to do something with it?

I know enough to know that I should give you a:

Tested-by: Alyssa Ross <hi@alyssa.is>

> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 839373451c2b7d..9d61c027185141 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1010,9 +1010,6 @@ static int loop_configure(struct loop_device *lo, f=
mode_t mode,
>  	/* This is safe, since we have a reference from open(). */
>  	__module_get(THIS_MODULE);
>
> -	/* suppress uevents while reconfiguring the device */
> -	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> -
>  	/*
>  	 * If we don't hold exclusive handle for the device, upgrade to it
>  	 * here to avoid changing device under exclusive owner.
> @@ -1067,6 +1064,9 @@ static int loop_configure(struct loop_device *lo, f=
mode_t mode,
>  		}
>  	}
>
> +	/* suppress uevents while reconfiguring the device */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> +
>  	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
>  	set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) !=3D 0);
>
> @@ -1109,17 +1109,17 @@ static int loop_configure(struct loop_device *lo,=
 fmode_t mode,
>  	if (partscan)
>  		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>
> +	/* enable and uncork uevent now that we are done */
> +	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> +
>  	loop_global_unlock(lo, is_loop);
>  	if (partscan)
>  		loop_reread_partitions(lo);
> +
>  	if (!(mode & FMODE_EXCL))
>  		bd_abort_claiming(bdev, loop_configure);
>
> -	error =3D 0;
> -done:
> -	/* enable and uncork uevent now that we are done */
> -	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> -	return error;
> +	return 0;
>
>  out_unlock:
>  	loop_global_unlock(lo, is_loop);
> @@ -1130,7 +1130,7 @@ static int loop_configure(struct loop_device *lo, f=
mode_t mode,
>  	fput(file);
>  	/* This is safe: open() is still holding a reference. */
>  	module_put(THIS_MODULE);
> -	goto done;
> +	return error;
>  }
>
>  static void __loop_clr_fd(struct loop_device *lo, bool release)

--br25mg72tdjmjgjf
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEH9wgcxqlHM/ARR3h+dvtSFmyccAFAmQVGMsACgkQ+dvtSFmy
ccA1pw//S4bRusb/civnT/kkdp/QLhDLd2sRRJX5sfU2YjNCr9kWM9CciuLrFAPE
lVLpUpygKWVMWwby2WcLD02EVVLqzyYRMt+bPl8ynA4tZsxryC9Do9q87vB+yWLh
z9gko75rR1rauESCWq12Kbvp0UQrm98PsWm5bqZrt+1lugnJH+ovvxpZ7NlVB9TO
zO4RW2BCfPP1mrNjcKOQCw0tC+3HRXqbMMTnHvKgJA+kyW3qf3zLX7RWlPm9+WFI
glxOHwQgnopBjHVYdxkgqIBpkS8/Kh+ellFxspJkpT0YDZHmqPmAw/SAdCUrfbk/
oVKyiSE4NkZ8h0MxnMta2RyuMLrRNWgTO9twEw2V1Kt/z/U3jCFDz4fw1Nj8iKNM
TouZe71Xa6KPUPCTkypWT3pDEDbQ/JM5uNc45XS7wi6LfaMa1d/BwBXstDH1+brY
TEssldSfMMvp1fU4kZ3RNz8nS5Xx29jFEQ+qsZoVY7JGIDHV3g3XHFL7s3GZLjtF
m5/ZMyJma/9KEzi2UXAI3I4dcACUrZQ3MUwo1TKdpkPIB/bx5m3ja2NyQeZAoZvR
3GoFd8ShR8FNdYPy+SK2SIbFBoy/OveRgw053U3VdGUEPKwqZlJynT2QFpnuvzqq
WEP/3aQ2AlFa1pFjFJrGsbvtu+2ZUlaSNS9V7ndrrGTISsaKfKU=
=MXQQ
-----END PGP SIGNATURE-----

--br25mg72tdjmjgjf--