[GIT PULL] file locking and writeback error handling patches for v4.13

[GIT PULL] file locking and writeback error handling patches for v4.13

[Jeff Layton]

[-- 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 --]

Re: [GIT PULL] file locking and writeback error handling patches for v4.13

[Linus Torvalds]

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

Re: [GIT PULL] file locking and writeback error handling patches for v4.13

[Jeff Layton]

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>