linux-ext4 - Re: [PATCH v2] fs: Convert return type int to vm_fault

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:   Fri, 31 Aug 2018 12:58:44 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Souptick Joarder <jrdr.linux@...il.com>,
        konishi.ryusuke@....ntt.co.jp, viro@...iv.linux.org.uk,
        adilger.kernel@...ger.ca, axboe@...nel.dk, darrick.wong@...cle.com,
        ebiggers@...gle.com, pombredanne@...b.com, agruenba@...hat.com,
        gregkh@...uxfoundation.org, kemi.wang@...el.com,
        willy@...radead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-nilfs@...r.kernel.org
Subject: Re: [PATCH v2] fs: Convert return type int to vm_fault_t

On Thu, Aug 30, 2018 at 04:35:21PM -0700, Andrew Morton wrote:
> 
> The v1->v2 delta (below) reveals unchangelogged ext4 changes?

Souptick, please don't make unrelated changes in a vm_fault_t patch.

Especially please don't make whitespace changes that will cause
checkpatch.pl to whine about line lengths longer than 80 characters.
There's a *reason* for the original indentation.

> @@ -6239,8 +6237,7 @@ retry_alloc:
>  		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
>  	}
>  	ext4_journal_stop(handle);
> -	if (err == -ENOSPC &&
> -		ext4_should_retry_alloc(inode->i_sb, &retries))
> +	if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry_alloc;
>  out_ret:
>  	ret = block_page_mkwrite_return(err);

The fact that this is not a non-trivials set of changes means anything
to make reviews harder is really not appreciated.

Also, the fact that the patch series involves multiple file system is
a massive pain.  It means I'm going to have to do a separate
regression test --- or preferably, I would ask *you* to run a file
system regression test[1] --- to make sure what is *not* a trivial
patch doesn't break things.  Also, it means that this patch series is
going to get more complicated to get into kernel, and we may have to
deal with patch conflicts if this goes in via some third party tree
(such as Andrew's tree).

[1] https:/thunk.org/gce-xfstests

One way to make life easier is to add the new function with the new
interface first, and then wait a release cycle, and then move file
systems over in independant patches.  

					- Ted