Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 6 Apr 2022 21:36:34 +0300
From:   Maxim Devaev <mdevaev@gmail.com>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     linux-usb@vger.kernel.org, Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Cai Huoqing <caihuoqing@baidu.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via
 configfs
Message-ID: <20220406213634.104cae45@reki>
In-Reply-To: <Yk3TLPKyaQDsnuD4@rowland.harvard.edu>
References: <20220406092445.215288-1-mdevaev@gmail.com>
        <Yk2wvhSTMKTLFK6c@rowland.harvard.edu>
        <20220406195234.4f63cb4a@reki>
        <Yk3TLPKyaQDsnuD4@rowland.harvard.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

=D0=92 Wed, 6 Apr 2022 13:51:40 -0400
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, Apr 06, 2022 at 07:52:34PM +0300, Maxim Devaev wrote:
> > > It's not clear to me how breaking I/O operations allows you to do a=20
> > > "force eject".  It seems that what you would need is something like=20
> > > fsg_store_file() that omits the curlun->prevent_medium_removal check.
> > > Interrupting a lengthy I/O operation doesn't really have anything to =
do=20
> > > with this. =20
> >=20
> > Perhaps I chose the wrong path, it's just how my userspace code works n=
ow.
> > If the drive is connected to a Linux host, then in order to clear
> > the "file" and extract the image, I sent a SIGUSR1 signal to the "file-=
storage"
> > thread. This interrupted long IO operations, reset curlun->prevent_medi=
um_removal
> > and I got the ability to extract. =20
>=20
> Oh, I see.  That's kind of an unintended side effect of not calling=20
> raise_exception().
>=20
> And while it does interrupt long I/O operations, it does so in=20
> non-sanctioned way.  To the host it will appear as though the gadget's=20
> firmware has crashed, since the gadget will stop sending or receiving=20
> data.  Eventually the host will time out and reset the gadget.
>=20
> Maybe that's the sort of thing you want, but I rather doubt it.

It's hard to say how it actually should work in case of force removing.
At least the currect approach with SIGUSR1 is really working on thousands
systems and with Linux, Mac and Windows. I believe that the criterion
of the experiment is quite important here. I know of several other utilities
that use SIGUSR1 for similar purposes.

> > Will masking the curlun->prevent_medium_removal flag be enough? =20
>=20
> I think so.  But it will be blocked to some extent by long-running I/O=20
> operations, because those operations acquire the filesem rw-semaphore=20
> for reading.
>=20
> More precisely, each individual command holds the rw-semaphore.  But the=
=20
> semaphore is dropped between commands, and a long-running I/O operation=20
> typically consists of many separate commands.  So the blocking may be=20
> acceptable.

It is very important for KVM-over-IP to be able to command "turn it off imm=
ediately".
In this context, I would prefer "break_io" rather than "allow_force_remove".

> > > You should not call send_sig_info() directly; instead call=20
> > > raise_exception().  It already does the work you need (including some=
=20
> > > things you left out). =20
> >=20
> > raise_exception() assumes the setting of a new state, and I did not wan=
t to do this,
> > since the same does not happen when throwing a signal from userspace. =
=20
>=20
> Userspace isn't supposed to send the USR1 signal, only the INT, TERM, or=
=20
> KILL signals.  USR1 is supposed to be reserved for the driver's internal=
=20
> use.  Unfortunately, AFAIK there's no way to allow the driver to send a=20
> signal to itself without also allowing the signal to be sent by=20
> userspace.  :-(

It's funny that you actually helped me solve my problem thanks to this undo=
cumented
behavior. If it were not for the ability to send a signal, I would not be a=
ble to make
the necessary code, and my software would always be waiting for the complet=
ion of IO.

So here I am grateful to you - I didn't have to patch the kernel a few year=
s ago,
and now I just want to turn it into a clear feature :)

Given the needs of the userspace code, maybe the suggested "break_io"
would be the best choice?

> And sending the signal _does_ set a new state, whether you intended to=20
> or not.  Although in this case, the new state is always the same as the=20
> old state, i.e., FSG_STATE_NORMAL.

So I could call raise_exception(fsg->common, FSG_STATE_NORMAL) instead of s=
ending
the signal from break_io handler. There will be a slight difference
in exception_req_tag and exception_arg, but it does not seem to cause any s=
ide effects.
Please correct me if I'm wrong.