linux-kernel - Re: write_cache

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] [day] [month] [year] [list]

Message-ID: <20111109164537.GA7495@quack.suse.cz>
Date:	Wed, 9 Nov 2011 17:45:37 +0100
From:	Jan Kara <jack@...e.cz>
To:	Phillip Susi <psusi@....rr.com>
Cc:	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: write_cache_pages inefficiency

On Sun 06-11-11 16:48:33, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I've read over write_cache_pages() in page-writeback.c, and related
> writepages() functions, and it seems to me that it suffers from a
> performance problem whenever an fsync is done on a file and some of
> its pages have already begun writeback.  The comment in the code says:
> 
>  * If a page is already under I/O, write_cache_pages() skips it, even
>  * if it's dirty.  This is desirable behaviour for memory-cleaning
> writeback,
>  * but it is INCORRECT for data-integrity system calls such as
> fsync().  fsync()
>  * and msync() need to guarantee that all the data which was dirty at
> the time
>  * the call was made get new I/O started against them.  If
> wbc->sync_mode is
>  * WB_SYNC_ALL then we were called for data integrity and we must wait for
>  * existing IO to complete.
> 
> Based on this, I would expect the function to wait for an existing
> write to complete only if the page is also dirty.  Instead, it waits
> for existing page writes to complete regardless of the dirty bit.
  Are you sure? I can see in the code:
                        lock_page(page);
                        if (unlikely(page->mapping != mapping)) {
continue_unlock:
                                unlock_page(page);
                                continue;
                        }
                        if (!PageDirty(page)) {
                                /* someone wrote it for us */
                                goto continue_unlock;
                        }
                        if (PageWriteback(page)) {
                                if (wbc->sync_mode != WB_SYNC_NONE)
                                        wait_on_page_writeback(page);
                                else
                                        goto continue_unlock;
                        }
  So we skip clean pages...

> Additionally, it does each wait serially, so if you are trying to
> fsync 1000 dirty pages, and the first 10 are already being written
> out, the thread will block on each of those 10 pages write completion
> before it begins queuing any new writes.
  Yes, this is correct.

> Instead, shouldn't it go ahead and initiate pagewrite on all pages not
> already being written, and then come back and wait on those that were
> already in flight to complete, then initiate a second write on them if
> they are dirty?
  Well, if you can *demonstrate* with real numbers it has performance benefit
we could do it. But it's not clear there will be any benefit - skipping
pages which need writing can introduce additional seeks to the IO stream
and that is costly - sometimes much more costly than just waiting for IO to
complete...

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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/