All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@redhat.com>
To: linux-mm <linux-mm@kvack.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	 LKML <linux-kernel@vger.kernel.org>
Cc: lsf-pc <lsf-pc@lists.linuxfoundation.org>, Neil Brown <neilb@suse.de>
Subject: [LSF/MM TOPIC] do we really need PG_error at all?
Date: Sun, 26 Feb 2017 09:42:44 -0500	[thread overview]
Message-ID: <1488120164.2948.4.camel@redhat.com> (raw)

Proposing this as a LSF/MM TOPIC, but it may turn out to be me just not
understanding the semantics here.

As I was looking into -ENOSPC handling in cephfs, I noticed that
PG_error is only ever tested in one place [1] __filemap_fdatawait_range,
which does this:

	if (TestClearPageError(page))
		ret = -EIO;

This error code will override any AS_* error that was set in the
mapping. Which makes me wonder...why don't we just set this error in the
mapping and not bother with a per-page flag? Could we potentially free
up a page flag by eliminating this?

The main argument I could see for keeping it is that removing it might
subtly change the behavior of sync_file_range if you have tasks syncing
different ranges in a file concurrently. I'm not sure if that would
break any guarantees though.

Even if we do need it, I think we might need some cleanup here anyway. A
lot of readpage operations end up setting that flag when they hit an
error. Isn't it wrong to return an error on fsync, just because we had a
read error somewhere in the file in a range that was never dirtied?

--
[1]: there is another place in f2fs, but it's more or less equivalent to
the call site in __filemap_fdatawait_range.

-- 
Jeff Layton <jlayton@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jlayton@redhat.com>
To: linux-mm <linux-mm@kvack.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Cc: lsf-pc <lsf-pc@lists.linuxfoundation.org>, Neil Brown <neilb@suse.de>
Subject: [LSF/MM TOPIC] do we really need PG_error at all?
Date: Sun, 26 Feb 2017 09:42:44 -0500	[thread overview]
Message-ID: <1488120164.2948.4.camel@redhat.com> (raw)

Proposing this as a LSF/MM TOPIC, but it may turn out to be me just not
understanding the semantics here.

As I was looking into -ENOSPC handling in cephfs, I noticed that
PG_error is only ever tested in one place [1] __filemap_fdatawait_range,
which does this:

	if (TestClearPageError(page))
		ret = -EIO;

This error code will override any AS_* error that was set in the
mapping. Which makes me wonder...why don't we just set this error in the
mapping and not bother with a per-page flag? Could we potentially free
up a page flag by eliminating this?

The main argument I could see for keeping it is that removing it might
subtly change the behavior of sync_file_range if you have tasks syncing
different ranges in a file concurrently. I'm not sure if that would
break any guarantees though.

Even if we do need it, I think we might need some cleanup here anyway. A
lot of readpage operations end up setting that flag when they hit an
error. Isn't it wrong to return an error on fsync, just because we had a
read error somewhere in the file in a range that was never dirtied?

--
[1]: there is another place in f2fs, but it's more or less equivalent to
the call site in __filemap_fdatawait_range.

-- 
Jeff Layton <jlayton@redhat.com>

             reply	other threads:[~2017-02-26 14:42 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-26 14:42 Jeff Layton [this message]
2017-02-26 14:42 ` [LSF/MM TOPIC] do we really need PG_error at all? Jeff Layton
2017-02-26 17:10 ` James Bottomley
2017-02-26 17:10   ` James Bottomley
2017-02-26 21:03   ` NeilBrown
2017-02-26 22:43     ` Jeff Layton
2017-02-26 22:43       ` Jeff Layton
2017-02-26 23:30     ` James Bottomley
2017-02-26 23:57       ` Jeff Layton
2017-02-26 23:57         ` Jeff Layton
2017-02-27  0:27       ` NeilBrown
2017-02-27 15:07         ` Jeff Layton
2017-02-27 15:07           ` Jeff Layton
2017-02-27 22:51           ` Andreas Dilger
2017-02-27 23:02             ` Jeff Layton
2017-02-27 23:02               ` Jeff Layton
2017-02-27 23:32             ` NeilBrown
2017-02-28  1:11               ` [Lsf-pc] " Jeff Layton
2017-02-28  1:11                 ` Jeff Layton
2017-02-28  1:11                 ` Jeff Layton
2017-02-28 10:12                 ` Boaz Harrosh
2017-02-28 10:12                   ` Boaz Harrosh
2017-02-28 11:32                   ` Jeff Layton
2017-02-28 11:32                     ` Jeff Layton
2017-02-28 20:45                 ` NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1488120164.2948.4.camel@redhat.com \
    --to=jlayton@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=lsf-pc@lists.linuxfoundation.org \
    --cc=neilb@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.