* [GIT PULL] file locking and writeback error handling patches for v4.13
@ 2017-07-03 13:58 Jeff Layton
2017-07-05 18:10 ` Linus Torvalds
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Layton @ 2017-07-03 13:58 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Kees Cook, LKML, linux-fsdevel
[-- Attachment #1: Type: text/plain, Size: 6725 bytes --]
The following changes since commit c86daad2c25bfd4a33d48b7691afaa96d9c5ab46:
Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input (2017-05-26 16:45:13 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git tags/for-linus-v4.13
for you to fetch changes up to d997cba55291de59ed4a107394ac2e61b6379ac2:
btrfs: minimal conversion to errseq_t writeback error reporting on fsync (2017-07-03 08:41:09 -0400)
----------------------------------------------------------------
File locking and writeback error handling patches for v4.13
First, this PR contains a small pile of file-locking and fcntl.c related
changes (some of which have also been pulled into Al's tree).
After that, there is a pile of patches to clean up the handling of
writeback errors from the pagecache. That part strays outside the usual
area of my PRs. The changes touch a lot of areas though, so I think it's
best to pull them in as a set rather than feeding them via subsystem
trees.
In addition to cleaning up races and other bugs, new infrastructure is
added to handle tracking errors that occur during data writeback to
allow for more reliable reporting of those errors to
fsync/fdatasync/msync. For now, only ext*, xfs, btrfs and some other
simple block-based filesystems (FAT, minix, etc.) are converted to the
new behavior. We'll be converting others over the next few releases.
Note that there may be some trivial merge conflicts between this and
Kees Cook's KSPP PR. They should be simple to resolve, but let me know
if they aren't.
----------------------------------------------------------------
Benjamin Coddington (3):
fs/locks: Use allocation rather than the stack in fcntl_getlk()
fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks
staging/lustre, 9p, ceph, cifs, dlm: negate remote pids for F_GETLK
Christoph Hellwig (2):
fs/locks: pass kernel struct flock to fcntl_getlk/setlk
fs/locks: don't mess with the address limit in compat_fcntl64
Dave Kleikamp (1):
JFS: do not ignore return code from write_one_page()
Jeff Layton (21):
fs/fcntl: return -ESRCH in f_setown when pid/pgid can't be found
mm: drop "wait" parameter from write_one_page()
mm: fix mapping_set_error call in me_pagecache_dirty
fs: remove call_fsync helper function
buffer: use mapping_set_error instead of setting the flag
fs: check for writeback errors after syncing out buffers in generic_file_fsync
buffer: set errors in mapping at the time that the error occurs
jbd2: don't clear and reset errors after waiting on writeback
mm: clear AS_EIO/AS_ENOSPC when writeback initiation fails
mm: don't TestClearPageError in __filemap_fdatawait_range
mm: clean up error handling in write_one_page
lib: add errseq_t type and infrastructure for handling it
fs: new infrastructure for writeback error handling and reporting
mm: set both AS_EIO/AS_ENOSPC and errseq_t in mapping_set_error
Documentation: flesh out the section in vfs.txt on storing and reporting writeback errors
dax: set errors in mapping when writeback fails
block: convert to errseq_t based writeback error tracking
fs: convert __generic_file_fsync to use errseq_t based reporting
ext4: use errseq_t based error handling for reporting data writeback errors
xfs: minimal conversion to errseq_t writeback error reporting
btrfs: minimal conversion to errseq_t writeback error reporting on fsync
Jiri Slaby (2):
fs/fcntl: f_setown, allow returning error
fs/fcntl: f_setown, avoid undefined behaviour
Mauro Carvalho Chehab (1):
fs: locks: Fix some troubles at kernel-doc comments
Documentation/filesystems/vfs.txt | 44 ++++-
MAINTAINERS | 6 +
drivers/dax/device.c | 1 +
drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
fs/9p/vfs_file.c | 2 +-
fs/block_dev.c | 3 +-
fs/btrfs/file.c | 13 +-
fs/buffer.c | 20 ++-
fs/ceph/locks.c | 2 +-
fs/cifs/cifssmb.c | 2 +-
fs/dax.c | 4 +-
fs/dlm/plock.c | 2 +-
fs/exofs/dir.c | 2 +-
fs/ext2/dir.c | 2 +-
fs/ext2/file.c | 5 +-
fs/ext4/fsync.c | 2 +-
fs/fcntl.c | 172 +++++++++++++-------
fs/file_table.c | 1 +
fs/fuse/file.c | 6 +-
fs/gfs2/lops.c | 2 +-
fs/jbd2/commit.c | 16 +-
fs/jfs/jfs_metapage.c | 7 +-
fs/jfs/jfs_metapage.h | 1 +
fs/libfs.c | 6 +-
fs/locks.c | 193 ++++++++++------------
fs/minix/dir.c | 2 +-
fs/open.c | 3 +
fs/sync.c | 2 +-
fs/sysv/dir.c | 2 +-
fs/ufs/dir.c | 2 +-
fs/xfs/xfs_file.c | 2 +-
include/linux/buffer_head.h | 1 +
include/linux/errseq.h | 19 +++
include/linux/fs.h | 80 +++++++--
include/linux/mm.h | 2 +-
include/linux/pagemap.h | 31 +++-
include/trace/events/filemap.h | 57 +++++++
ipc/shm.c | 2 +-
lib/Makefile | 2 +-
lib/errseq.c | 208 ++++++++++++++++++++++++
mm/filemap.c | 126 ++++++++++++--
mm/memory-failure.c | 2 +-
mm/page-writeback.c | 19 +--
net/socket.c | 3 +-
44 files changed, 806 insertions(+), 275 deletions(-)
create mode 100644 include/linux/errseq.h
create mode 100644 lib/errseq.c
--
Jeff Layton <jlayton@redhat.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 862 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PULL] file locking and writeback error handling patches for v4.13
2017-07-03 13:58 [GIT PULL] file locking and writeback error handling patches for v4.13 Jeff Layton
@ 2017-07-05 18:10 ` Linus Torvalds
2017-07-05 18:53 ` Jeff Layton
0 siblings, 1 reply; 3+ messages in thread
From: Linus Torvalds @ 2017-07-05 18:10 UTC (permalink / raw)
To: Jeff Layton; +Cc: Kees Cook, LKML, linux-fsdevel
On Mon, Jul 3, 2017 at 6:58 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> File locking and writeback error handling patches for v4.13
Yeah, I won't be pulling this.
It's a random collection of patches, and the ones I looked at looked
actively wrong.
For example, you add support for remote pid in one commit, and then in
the next commit you convert filesystems to use them.
Which means that in between, locking presumably doesn't work ata ll,
because the locks_translate_pid() function presumably now does the
wrong thing. It used to be conditional of fl_nspid, which hid that
issue, but that now goes away, and you rely on the sign of the pid
instead, but the sign hasn't been *changed* yet.
Now, it's entirely possible that I'm mis-reading all this, but it
means that I find even the file locking changes suspicious.
But that's not what kills this pull request for me.
No, what makes me go "No" is that it then mixes up the file locking
changes with completely unrelated mapping things.
And some of those look like obvious fixes that I have nothing at all against.
And then some just look stupid (but aren't):
- return ret;
+ err = filemap_check_errors(inode->i_mapping);
+ return ret ? ret : err;
Yeah, why is it pointlessly calling filemap_check_errors() even when
the value won't get used? It turns out that that's because
filemap_check_errors() also *clears* the pending errors, but that
isn't at all obvious from the code, so it definitely merits a comment
somewhere).
Related to that very fact, in other parts "filemap_check_errors()" is
literally used to clear the errors, and a comment *is* added there.
Which makes me think that maybe that function should just be renamed.
But.. Then you introduce the writeback error code which is big and new
and doesn't look wrong, but wasn't really described much in your pull
request, and just makes me go "this is all a surprise, and none of it
has anythign to do with file locking".
So together, all of these issues spell "no, I'm not happy to pull
this" to me. Maybe the code is all fine for various reasons, maybe I'm
mis-reading the non-bisectable semantic change to pid's, maybe it's
all good.
But even if the code is all good, I *really* want this as separate
pull requests that make sense on their own. One for file locking that
was unrelated to everything else, one for the trivial fixes, and one
for the whole new writeback error handling logic.
Linus
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PULL] file locking and writeback error handling patches for v4.13
2017-07-05 18:10 ` Linus Torvalds
@ 2017-07-05 18:53 ` Jeff Layton
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Layton @ 2017-07-05 18:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Kees Cook, LKML, linux-fsdevel, Benjamin Coddington
On Wed, 2017-07-05 at 11:10 -0700, Linus Torvalds wrote:
> On Mon, Jul 3, 2017 at 6:58 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > File locking and writeback error handling patches for v4.13
>
> Yeah, I won't be pulling this.
>
> It's a random collection of patches, and the ones I looked at looked
> actively wrong.
>
> For example, you add support for remote pid in one commit, and then in
> the next commit you convert filesystems to use them.
>
> Which means that in between, locking presumably doesn't work ata ll,
> because the locks_translate_pid() function presumably now does the
> wrong thing. It used to be conditional of fl_nspid, which hid that
> issue, but that now goes away, and you rely on the sign of the pid
> instead, but the sign hasn't been *changed* yet.
>
(cc'ing Ben)
Good point. I think the end result is where we want to be, but it might
be best to squash the last two locking patches there into a single patch
to ensure we don't have a regression at between them.
Ben, any objections to doing that?
> Now, it's entirely possible that I'm mis-reading all this, but it
> means that I find even the file locking changes suspicious.
>
> But that's not what kills this pull request for me.
>
> No, what makes me go "No" is that it then mixes up the file locking
> changes with completely unrelated mapping things.
>
> And some of those look like obvious fixes that I have nothing at all against.
>
> And then some just look stupid (but aren't):
>
> - return ret;
> + err = filemap_check_errors(inode->i_mapping);
> + return ret ? ret : err;
>
> Yeah, why is it pointlessly calling filemap_check_errors() even when
> the value won't get used? It turns out that that's because
> filemap_check_errors() also *clears* the pending errors, but that
> isn't at all obvious from the code, so it definitely merits a comment
> somewhere).
>
> Related to that very fact, in other parts "filemap_check_errors()" is
> literally used to clear the errors, and a comment *is* added there.
> Which makes me think that maybe that function should just be renamed.
>
Fair enough -- I'll toss in a patch to rename it to
filemap_check_and_clear_errors() early in the series.
> But.. Then you introduce the writeback error code which is big and new
> and doesn't look wrong, but wasn't really described much in your pull
> request, and just makes me go "this is all a surprise, and none of it
> has anythign to do with file locking".
>
> So together, all of these issues spell "no, I'm not happy to pull
> this" to me. Maybe the code is all fine for various reasons, maybe I'm
> mis-reading the non-bisectable semantic change to pid's, maybe it's
> all good.
>
> But even if the code is all good, I *really* want this as separate
> pull requests that make sense on their own. One for file locking that
> was unrelated to everything else, one for the trivial fixes, and one
> for the whole new writeback error handling logic.
>
> Linus
Ok, thanks for looking -- that's all fair criticism.
I'll clean this up into separate branches and send them as 3 individual
PRs. I should be able to get them sent out tomorrow or Friday.
Thanks,
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-05 18:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 13:58 [GIT PULL] file locking and writeback error handling patches for v4.13 Jeff Layton
2017-07-05 18:10 ` Linus Torvalds
2017-07-05 18:53 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).