Re: [PATCH] usb: gadget: f_mass_storage: break IO operations via configfs

[Maxim Devaev <mdevaev@gmail.com>]

В Fri, 8 Apr 2022 10:59:45 -0400
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, Apr 07, 2022 at 08:47:13PM +0300, Maxim Devaev wrote:
> > В Thu, 7 Apr 2022 12:06:01 -0400
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> >   
> > > On Wed, Apr 06, 2022 at 09:36:34PM +0300, Maxim Devaev wrote:  
> > > > В 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 
> > > > > > > "force eject".  It seems that what you would need is something like 
> > > > > > > fsg_store_file() that omits the curlun->prevent_medium_removal check.
> > > > > > > Interrupting a lengthy I/O operation doesn't really have anything to do 
> > > > > > > with this.      
> > > > > > 
> > > > > > Perhaps I chose the wrong path, it's just how my userspace code works now.
> > > > > > 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_medium_removal
> > > > > > and I got the ability to extract.      
> > > > > 
> > > > > Oh, I see.  That's kind of an unintended side effect of not calling 
> > > > > raise_exception().
> > > > > 
> > > > > And while it does interrupt long I/O operations, it does so in 
> > > > > non-sanctioned way.  To the host it will appear as though the gadget's 
> > > > > firmware has crashed, since the gadget will stop sending or receiving 
> > > > > data.  Eventually the host will time out and reset the gadget.
> > > > > 
> > > > > 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.    
> > > 
> > > This merely means that the current unintended behavior of userspace USR1 
> > > signals must not be changed.  But it doesn't mean you have to continue 
> > > to rely on that behavior; you can implement something better.  
> > 
> > So I suggest break_io :) I haven't come up with anything better.  
> 
> But breaking an I/O doesn't do all that you want.  That is, interrupting an 
> I/O request (causing an executing command to terminate early) doesn't in 
> itself change the prevent/allow status.  Those are two separate operations.  
> The fact that sending a USR1 signal does both is merely a coincidence.
> 
> Furthermore, it's not clear just what you mean when you say KVM needs to 
> "turn it off immediately".  How soon is "immediately"?  Even a USR1 signal 
> doesn't work instantaneously.  You may find that a forced eject without an 
> I/O interruption works quickly enough.

Yes, you're right. I need to focus on forced eject operation.

> > > > > > Will masking the curlun->prevent_medium_removal flag be enough?      
> > > > > 
> > > > > I think so.  But it will be blocked to some extent by long-running I/O 
> > > > > operations, because those operations acquire the filesem rw-semaphore 
> > > > > for reading.
> > > > > 
> > > > > More precisely, each individual command holds the rw-semaphore.  But the 
> > > > > semaphore is dropped between commands, and a long-running I/O operation 
> > > > > typically consists of many separate commands.  So the blocking may be 
> > > > > acceptable.    
> > > > 
> > > > It is very important for KVM-over-IP to be able to command "turn it off immediately".    
> > > 
> > > Why is this?  A lot of actual devices (DVD drives, for instance) don't 
> > > give you the ability to eject the media when the host has prevented it.  
> > > Why should f-mass-storage be different?  
> > 
> > The DVD drive has the ability to physically eject the disc.  
> 
> You mean by sticking an unfolded paperclip into the manual-eject hole?

Yes.

> >  It's not too good
> > for the drive itself, but it's just there. We can also urgently remove
> > the USB flash drive.  
> 
> Removing a USB flash drive is not a media-eject operation; it's a 
> disconnect operation.  (That is, it removes the entire device, not just the 
> media.)  By contrast, taking an SD card out from a USB card reader _is_ an 
> example of a media ejection.  But card readers do not claim to support the 
> prevent/allow mechanism.
> 
> > At least there is one situation where the behavior of f_mass_storage differs
> > from the behavior of a real drive. What happens when you click on the physical
> > "eject" button?  
> 
> If the host has prevented ejection, nothing happens.  Otherwise the disc 
> gets ejected.
> 
> > Yes, the OS can block this, but the problem is that we don't have
> > an "eject" here.  
> 
> What do you mean?  Writing an empty string to the sysfs "file" attribute 
> is the virtual analog of pressing the eject button.

But I can't eject the disc event it's not mounted on Linux host. It seems to me
it differs from the real drive behavior.

> ...

I have reflected on the rest of your arguments and changed my mind.
I think that "forced_eject" for a specific lun without interrupting operations would
really be the best solution. I wrote a simple patch and tested it, everything seems
to work. What do you think about something like this?


static ssize_t fsg_lun_opts_forced_eject_store(struct config_item *item,
                                               const char *page, size_t len)
{
        struct fsg_lun_opts *opts = to_fsg_lun_opts(item);
        struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent);
        int ret;

        opts->lun->prevent_medium_removal = 0;
        ret = fsg_store_file(opts->lun, &fsg_opts->common->filesem, "", 0);
        return ret < 0 ? ret : len;
}

CONFIGFS_ATTR_WO(fsg_lun_opts_, forced_eject);


If you find this acceptable, I will test this patch on my users to make sure
that its behavior meets our expectations.