linux-kernel - Re: [PATCH 1/1] fs/mpage.c: forgotten WRITE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACZ9PQUQX-sSNo+z0LB0Tefq9pSHFr6GjKg2tWzdAtw3OYYDsg@mail.gmail.com>
Date:	Fri, 14 Mar 2014 23:07:04 +0900
From:	Roman Peniaev <r.peniaev@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, Jan Kara <jack@...e.cz>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH 1/1] fs/mpage.c: forgotten WRITE_SYNC in case of data
 integrity write

Hello, Tejun.

On Fri, Mar 14, 2014 at 10:07 PM, Tejun Heo <tj@...nel.org> 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@...il.com>
>> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/