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]

Date:	Tue, 18 Feb 2014 15:59:45 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Roman Pen <r.peniaev@...il.com>
Cc:	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

On Sun, 16 Feb 2014 11:54:28 +0900 Roman Pen <r.peniaev@...il.com> wrote:

> In case of wbc->sync_mode == WB_SYNC_ALL we need to do data integrity write,
> thus mark request as WRITE_SYNC.

gargh, the documentation for this stuff is useless.

What do REQ_SYNC and REQ_NOIDLE actually *do*?

For mpage writes, REQ_NOIDLE appears to be incorrect - we very much
expect that there will be more writes and that they will be contiguous
with this one.  But we won't be waiting on this write before submitting
more writes, so perhaps REQ_NOIDLE is at least harmless.

I dunno about REQ_SYNC - it requires delving into the bowels of CFQ
and we shouldn't need to do that.

Jens.  Help.  How is a poor kernel reader supposed to work this out?

> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -462,6 +462,7 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
>  	struct buffer_head map_bh;
>  	loff_t i_size = i_size_read(inode);
>  	int ret = 0;
> +	int wr = (wbc->sync_mode == WB_SYNC_ALL ?  WRITE_SYNC : WRITE);
>  
>  	if (page_has_buffers(page)) {
>  		struct buffer_head *head = page_buffers(page);
> @@ -570,7 +571,7 @@ page_is_mapped:
>  	 * This page will go to BIO.  Do we need to send this BIO off first?
>  	 */
>  	if (bio && mpd->last_block_in_bio != blocks[0] - 1)
> -		bio = mpage_bio_submit(WRITE, bio);
> +		bio = mpage_bio_submit(wr, bio);
>  
>  alloc_new:
>  	if (bio == NULL) {
> @@ -587,7 +588,7 @@ alloc_new:
>  	 */
>  	length = first_unmapped << blkbits;
>  	if (bio_add_page(bio, page, length, 0) < length) {
> -		bio = mpage_bio_submit(WRITE, bio);
> +		bio = mpage_bio_submit(wr, bio);
>  		goto alloc_new;
>  	}
>  
> @@ -620,7 +621,7 @@ alloc_new:
>  	set_page_writeback(page);
>  	unlock_page(page);
>  	if (boundary || (first_unmapped != blocks_per_page)) {
> -		bio = mpage_bio_submit(WRITE, bio);
> +		bio = mpage_bio_submit(wr, bio);
>  		if (boundary_block) {
>  			write_boundary_block(boundary_bdev,
>  					boundary_block, 1 << blkbits);
> @@ -632,7 +633,7 @@ alloc_new:
>  
>  confused:
>  	if (bio)
> -		bio = mpage_bio_submit(WRITE, bio);
> +		bio = mpage_bio_submit(wr, bio);
>  
>  	if (mpd->use_writepage) {
>  		ret = mapping->a_ops->writepage(page, wbc);
> @@ -688,8 +689,11 @@ mpage_writepages(struct address_space *mapping,
>  		};
>  
>  		ret = write_cache_pages(mapping, wbc, __mpage_writepage, &mpd);
> -		if (mpd.bio)
> -			mpage_bio_submit(WRITE, mpd.bio);
> +		if (mpd.bio) {
> +			int wr = (wbc->sync_mode == WB_SYNC_ALL ?
> +				  WRITE_SYNC : WRITE);
> +			mpage_bio_submit(wr, mpd.bio);
> +		}
>  	}
>  	blk_finish_plug(&plug);
>  	return ret;
> @@ -706,8 +710,11 @@ int mpage_writepage(struct page *page, get_block_t get_block,
>  		.use_writepage = 0,
>  	};
>  	int ret = __mpage_writepage(page, wbc, &mpd);
> -	if (mpd.bio)
> -		mpage_bio_submit(WRITE, mpd.bio);
> +	if (mpd.bio) {
> +		int wr = (wbc->sync_mode == WB_SYNC_ALL ?
> +			  WRITE_SYNC : WRITE);
> +		mpage_bio_submit(wr, mpd.bio);
> +	}
>  	return ret;
>  }
>  EXPORT_SYMBOL(mpage_writepage);

--
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/