2011/11/2 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> On Wed, 2 Nov 2011, Yuping Luo wrote:
>
>> On Sat, Oct 29, 2011 at 5:48 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>> > On Fri, 28 Oct 2011, Felipe Balbi wrote:
>> >
>> >> On Fri, Oct 28, 2011 at 03:22:16PM -0400, Alan Stern wrote:
>> >> > On Fri, 28 Oct 2011, Alan Stern wrote:
>> >> >
>> >> > > The important questions are:
>> >> > >
>> >> > > When are the right times to flush the page cache?
>> >> > >
>> >> > > Why should the MSC gadget have to worry about this in the
>> >> > > first place? That is, why doesn't the rest of the kernel
>> >> > > take care of this automatically?
>> >> >
>> >> > I just went back and looked at the SCSI spec. The description of START
>> >> > STOP UNIT says this:
>> >> >
>> >> > Targets that contain cache memory shall implicitly perform a
>> >> > SYNCHRONIZE CACHE command for the entire medium prior to
>> >> > executing the STOP UNIT command.
>> >> >
>> >> > That's pretty clear. Currently the do_start_stop() routine just calls
>> >> > fsg_lun_close(); it looks like we should put an fsg_lun_fsync_sub()
>> >> > call there too if the backing storage is not a regular file (and maybe
>> >> > even if it _is_ a regular file, if we want to obey the spec strictly).
>> >> >
>> >> > Then there won't be any need for fsg_lun_fsync_sub() in
>> >> > do_prevent_allow().
>> >>
>> >> Makes sense to me. Care to send a patch to be queued for this -rc cycle?
>> >
>> > If Yuping or Barry would like to do it, that's fine with me. Otherwise
>> > I can send a patch.
>> >
>> > Alan Stern
>> >
>> >
>> just revert my previous change, and add following, will test with
>> windows os and linux.
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index 52583a2..2f79416 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -1497,8 +1497,6 @@ static int do_prevent_allow(struct fsg_common *common)
>> return -EINVAL;
>> }
>>
>> - if (curlun->prevent_medium_removal && !prevent)
>> - fsg_lun_fsync_sub(curlun);
>> curlun->prevent_medium_removal = prevent;
>> return 0;
>> }
>> diff --git a/drivers/usb/gadget/file_storage.c
>> b/drivers/usb/gadget/file_storage.c
>> index 3ac4f51..9a7f7f5 100644
>> --- a/drivers/usb/gadget/file_storage.c
>> +++ b/drivers/usb/gadget/file_storage.c
>> @@ -1880,8 +1880,6 @@ static int do_prevent_allow(struct fsg_dev *fsg)
>> return -EINVAL;
>> }
>>
>> - if (curlun->prevent_medium_removal && !prevent)
>> - fsg_lun_fsync_sub(curlun);
>> curlun->prevent_medium_removal = prevent;
>> return 0;
>> }
>> diff --git a/drivers/usb/gadget/storage_common.c
>> b/drivers/usb/gadget/storage_common.c
>> index c7f291a..2cef9de 100644
>> --- a/drivers/usb/gadget/storage_common.c
>> +++ b/drivers/usb/gadget/storage_common.c
>> @@ -765,6 +765,7 @@ static void fsg_lun_close(struct fsg_lun *curlun)
>> {
>> if (curlun->filp) {
>> LDBG(curlun, "close backing file\n");
>> + fsg_lun_fsync_sub(curlun);
>> fput(curlun->filp);
>> curlun->filp = NULL;
>> }
>
> You know what? I finally remembered to look at the SCSI spec's
> description of PREVENT ALLOW MEDIUM REMOVAL:
>
> The prevention of medium removal shall begin when any initiator
> issues a PREVENT ALLOW MEDIUM REMOVAL command with a prevent
> bit of one (medium removal prevented). The prevention of medium
> removal for the logical unit shall terminate:
>
> a) after all initiators that have medium removal
> prevented issue PREVENT ALLOW MEDIUM REMOVAL commands
> with a prevent bit of zero, and the target has
> successfully performed a synchronize cache operation;
>
> b) upon the receipt of a BUS DEVICE RESET message from any
> initiator; or
>
> c) upon a hard RESET condition.
>
> The last line of a) indicates that we really do need to keep the
> fsg_lun_fsync_sub() calls where they are. We probably could skip the
> sync if the backing storage is a regular file, because in that case it
> shouldn't be removed until its filesystem is unmounted, and unmounting
> syncs the page cache.
Alan, after thinking in the recent several days, i think we both have
misunderstood the meaning of cache operation in the description.
"the target has successfully performed a synchronize cache operation",
for a real u-disk, the cache means HW cache on disk controller and
"PREVENT ALLOW MEDIUM REMOVAL" just means we can safely unplug the
disk after that. if we don't do sync and unplug the disk after doing
"PREVENT ALLOW MEDIUM REMOVAL", data will be wrong since HW cache is
not flushed into physical disk.
But for USB gadget, it is much different actually. in fact, page cache
for gadget file is a thing on target board and has nothing to do with
the PC, it is just transprant to PC at all. PC can think all data has
been in U-disk after they execute any read/write commands. no matter
data is in page cache or in gagdet device, it has been in the U-disk
to PC. so i think we can't do fsync in gadget in the case "PREVENT
ALLOW MEDIUM REMOVAL" , all data has been in target board(the U-disk),
either in target board memory and target board gagdet-specific file or
partition. Then the target board need to take over the responsiblity
to flush page cache into physical medium before users unplug the card
bound with gagdet.
actually, no matter a disk is bound with gadget or not, once it is
written by any program and any user on target board, users need to
realize a sync/eject is needed before unplugging it. that is the
meaning i said gadget is just a program to access a writeable vfs
node.
> Alan Stern
-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
