add comments [1] [2] [3] [4]...
2011/11/2 Barry Song <21cnbao@xxxxxxxxx>:
> 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[1].
[1] real U-disk will lose power and lose HW cache after unplugging
>
> But for USB gadget, it is much different actually. in fact, page cache[2]
> for gadget file is a thing on target board and has nothing to do with
[2]page cache on target board
> 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[3]. no matter
[3]after PC sync the page cache on PC's OS to the USB gadget.
> 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[4].
[4]gadget driver is same with other VFS users on target board.
>
>
>
>> 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
