Re: [PATCH 1/1] fs/mpage.c: forgotten WRITE_SYNC in case of data integrity write

[Roman Peniaev]

Hello, Tejun.

On Fri, Mar 14, 2014 at 10:07 PM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> Hello, Andrew.
>
> On Thu, Mar 13, 2014 at 02:34:56PM -0700, Andrew Morton wrote:
>> Jens isn't talking to us.  Tejun, are you able explain REQ_SYNC?
>
> It has nothing to do with data integrity.  It's just a hint telling
> the block layer that someone is waiting for the IO so it'd be a good
> idea to prioritize it.  For example, nothing visible to userland
> really waits for periodic writebacks, so we can delay their processing
> to prioritize, for example, READs triggered from a page fault, which
> is obviously causing userland visible latency.
>
> Block layer treats all READs as REQ_SYNC and also allows upper layers
> to mark some writes REQ_SYNC for cases where somebody is waiting for
> the write to complete for cases like flush(2).
>
>> From: Roman Pen <r.peniaev@xxxxxxxxx>
>> Subject: fs/mpage.c: forgotten WRITE_SYNC in case of data integrity write
>>
>> In case of wbc->sync_mode == WB_SYNC_ALL we need to do data integrity
>> write, thus mark request as WRITE_SYNC.
>
> So, at least this patch description is very misleading.  WRITE_SYNC
> has *NOTHING* to do with data integrity.  The only thing matters is
> whether somebody is waiting for its completion or not.

The thing is that I started investigating WB_SYNC_ALL and WRITE_SYNC
from the __filemap_fdatawrite_range call which has explicit comment:

 * If sync_mode is WB_SYNC_ALL then this is a "data integrity" operation, as
 * opposed to a regular memory cleansing writeback.

So, in the commit msg I want to say that when we are doing integrity write
(sync, fsync calls) we have to mark requests as WRITE_SYNC, and there
is only one path
(mpage writeback path)  where WRITE_SYNC is missed. That was the idea.
(not WRITE_SYNC is responsible for integrity write, but in case of
integrity write)

Seems the following message should be better:
When data inegrity operation (sync, fsync, fdatasync calls) happens
writeback control is set to WB_SYNC_ALL.
In that case all write requests are marked with WRITE_SYNC, but on
mpage writeback path
WRITE_SYNC is missed. This patch fixes this.

Is it ok, what do you think?

Also, could you please help me do understand how can I guarantee
integrity in case of block device with big volatile
cache and filesystem, which does not support REQ_FLUSH/FUA?

Even if I am doing fsync, block device will never receive any
indication, that these requests should really be stored on device,
or cache should be flushed right now (like REQ_FLUSH/FUA behaviour).

Seems it is impossible to do explicit block device cache flush if
filesystem does not care about it.

Thanks.

--
Roman

>
> Thanks.
>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/