Re: [PATCH 1/1] hfsplus: skip unnecessary volume header sync

[sougata santra <sougata@tuxera.com>]

On 07/19/2014 01:58 PM, Vyacheslav Dubeyko wrote:
>
> On Jul 18, 2014, at 1:49 PM, Sougata Santra wrote:
>
>> On Fri, 2014-07-18 at 13:01 +0400, Vyacheslav Dubeyko wrote:
>>> On Fri, 2014-07-18 at 11:35 +0300, Sougata Santra wrote:
>>>
>>> [snip]
>>>> 2) Also, there was a error in error propagation. It it also fixed in
>>>> this patch.
>>>> -->snip<--
>>>> if (!error)
>>>>         error2 = error;
>>>> -->snap<--
>>>>
>>>> 3) The disk is only flushed if there was no error. Previously it was
>>>> always flushed without checking the error.
>>>
>>> So, do you mean that filemap_write_and_wait() and hfsplus_submit_bio()
>>> doesn't request writing on a volume?
>> Yes it did, but it wrote in the page-cache ?.
>
> The hfsplus_submit_bio() makes bio allocation via bio_alloc() and, then,
> send request on write into elevator queue by means of submit_bio_wait().
> Moreover, submit_bio_wait() submits a bio, and wait until it completes.
> It means that we have changed block on volume after returning from
> hfsplus_submit_bio().

AFAIK, this is not always correct, we don't change the block on the
volume. The block device driver uses special command to initiate data
transfer with the disk controller. Then the disk controller uses DMA to
transfer the data and raises an interrupt that it is done with the data
transfer. That does not mean that the data is there is in the
non-volatile memory of the disk. If there is a write back cache in the
disk then the data is stored there. Then the disk controller uses its
own algorithm to write back data from its own cache to disk. The flush
operation sends an empty bio requesting flush of the write back cache.
My argument was when there was a problem of transferring the data into
disk cache, why bother to flush it. And if any data has already been
written to disk cache, the disk controller takes care and writes it
into non volatile memory, given that there is no power-off/plug-out
scenarios.

>
> The filemap_write_and_wait() calls do_writepages() finally. It calls
> mapping->a_ops->writepages(). So, it means that do_writepages() calls
> hfsplus_writepages(). As a result, finally, requests on write are placed in
> elevator queue. And, again, dirty pages of inode->i_mapping (page cache)
> will be on volume after returning from filemap_write_and_wait().
>
> This is my understanding of hfsplus_sync_fs() method's logic. Please,
> correct me if I am wrong.
>
> So, if you try to prevent from blkdev_issue_flush() then it doesn't mean
> that you prevent from writing special files and volume header on volume.
> I suppose that logic of writing in any case was special policy. Because
> fsck utility can help to us in some bad situations. This is my understanding
> of this method's logic.

Please don't get incorrect. I am not sure if should be done or not.
Obviously if it is not the best thing to do, we won't do it :-).

>
> Thanks,
> Vyacheslav Dubeyko.
>

Thanks a lot,
   Sougata Santra