Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239985

Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Mon, Oct 5, 2015 at 10:18 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Your ext4 patch may well fix the issue, and be the right thing to do
> (_regardless_ of the revert, in fact - while it might make the revert
> unnecessary, it might also be a good idea even if we do revert).

Thinking a bit more about your patch, I actually am getting more and
more convinced that it's the wrong thing to do.

Why?

Because the whole "Setting copied=0 will tell the upper layers to
repeat the write" just seems a nasty layering violation, where the
low-level filesystem uses a magic return code to introduce a special
case at the upper layers. But the upper layers are actually already
*aware* of the special case, and in fact have a comment about it.

So I think that the whole "setting copied to 0" would actually make a
lot more sense in the *caller*. Just do it in generic_perform_write()
instead. Then all the special cases and the restarting is all
together.

What do you guys think? This basically simplifies the low-level
filesystem rules, and says:

 - the filesystem will only ever see a partial "->write_end()" for the
case where the page was up-to-date, so that there is no issue with
"oops, we now have part of the page that may not have been written at
all"

 - if the page wasn't up-to-date before, ->write_end() will either be
everything we said we'd do in ->write_begin(), or it will be nothing
at all.

Hmm? This would seem to keep the special cases at the right layer, and
actually allow low-level filesystems to simplify things (ie the
special "copied = 0" special case in ext4 goes away.

The ext4 side still worries me, though. You made that
"page_zero_new_buffers()" conditional on "copied" being non-zero, but
I'm not convinced it can be conditional. Even if we retry, that retry
may end up failing (for example, because the source isn't mapped, so
we return -EFAULT rather than re-doing the write), but we have those
new buffers that got allocated in write_begin(), and now nobody has
ever written any data to them at all, so they have random stale
contents.

So I do think this needs more thought. Or at least somebody should
explain to me better why it's all ok.

I'm attaching the "copied = 0" special case thing at the
generic_perform_write() level as a patch for comments. But for now I
still think that reverting would seem to be the safer thing (which
still possibly leaves things buggy with a racy unmap, but at least
it's the old bug that we've never hit in practice).

Dave? Ted? Comments?

               Linus

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Theodore Ts'o <tytso@mit.edu> - 2015-10-05 17:30 +0200
  Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 18:00 +0200
    Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Dave Hansen <dave.hansen@linux.intel.com> - 2015-10-05 18:30 +0200
      Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 22:30 +0200
        Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Dave Hansen <dave.hansen@linux.intel.com> - 2015-10-05 22:50 +0200
          Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-05 23:20 +0200
            Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-06 00:00 +0200
              Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Dave Hansen <dave.hansen@linux.intel.com> - 2015-10-06 01:40 +0200
                Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-06 11:10 +0200
        Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal "H. Peter Anvin" <hpa@zytor.com> - 2015-10-05 22:50 +0200
          Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Ingo Molnar <mingo@kernel.org> - 2015-10-06 10:00 +0200
            Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-06 11:20 +0200
              Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Ingo Molnar <mingo@kernel.org> - 2015-10-06 11:30 +0200
                Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-06 15:30 +0200
                Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Ingo Molnar <mingo@kernel.org> - 2015-10-06 15:50 +0200
  Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Dave Hansen <dave.hansen@linux.intel.com> - 2015-10-05 18:10 +0200
  Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Dave Hansen <dave.hansen@intel.com> - 2015-10-05 20:10 +0200
    Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Theodore Ts'o <tytso@mit.edu> - 2015-10-07 05:40 +0200
      Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/ data=journal Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-07 09:40 +0200
        Re: [REGRESSION] 998ef75ddb and aio-dio-invalidate-failure w/  data=journal Theodore Ts'o <tytso@mit.edu> - 2015-10-07 17:50 +0200
          [PATCH] ext4: use private version of page_zero_new_buffers() for data=journal mode Theodore Ts'o <tytso@mit.edu> - 2015-10-09 06:10 +0200
            Re: [PATCH] ext4: use private version of page_zero_new_buffers()  for data=journal mode "Leonid V. Fedorenchik" <Leonid.Fedorenchik@paragon-software.com> - 2015-10-13 08:40 +0200
            Re: [PATCH] ext4: use private version of page_zero_new_buffers() for  data=journal mode Jan Kara <jack@suse.cz> - 2015-10-15 13:20 +0200

csiph-web