Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Jan Kara <jack@suse.cz>,
	viro@zeniv.linux.org.uk, bart.vanassche@wdc.com,
	ming.lei@redhat.com, tytso@mit.edu, darrick.wong@oracle.com,
	jikos@kernel.org, rjw@rjwysocki.net, pavel@ucw.cz,
	len.brown@intel.com, linux-fsdevel@vger.kernel.org,
	boris.ostrovsky@oracle.com, jgross@suse.com,
	todd.e.brandt@linux.intel.com, nborisov@suse.com,
	martin.petersen@oracle.com, ONeukum@suse.com,
	oleksandr@natalenko.name, oleg.b.antonyan@gmail.com,
	yu.chen.surf@gmail.com, dan.j.williams@intel.com,
	linux-pm@vger.kernel.org, linux-block@vger.kernel.org,
	linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 03/11] fs: add frozen sb state helpers
Date: Fri, 1 Dec 2017 12:47:24 +0100
Message-ID: <20171201114724.GC8365@quack2.suse.cz> (raw)
In-Reply-To: <20171130190548.GJ729@wotan.suse.de>

On Thu 30-11-17 20:05:48, Luis R. Rodriguez wrote:
> On Thu, Nov 30, 2017 at 06:13:10PM +0100, Jan Kara wrote:
> > ... I dislike the _by_user() suffix as there may be different places that
> > call freeze_super() (e.g. device mapper does this during some operations).
> > Clearly we need to distinguish "by system suspend" and "the other" cases.
> > So please make this clear in the naming.
> 
> Ah. How about sb_frozen_by_cb() ?

And what does 'cb' stand for? :)

> > In fact, what might be a cleaner solution is to introduce a 'freeze_count'
> > for superblock freezing (we already do have this for block devices). Then
> > you don't need to differentiate these two cases - but you'd still need to
> > properly handle cleanup if freezing of all superblocks fails in the middle.
> > So I'm not 100% this works out nicely in the end. But it's certainly worth
> > a consideration.
> 
> Ah, there are three important reasons for doing it the way I did it which are
> easy to miss, unless you read the commit log message very carefully.
> 
> 0) The ioctl interface causes a failure to be sent back to userspace if
> you issue two consecutive freezes, or two thaws. Ie, once a filesystem is
> frozen, a secondary call will result in an error. Likewise for thaw.

Yep. But also note that there's *another* interface to filesystem freezing
which behaves differently - freeze_bdev() (used internally by dm). That
interface uses the counter and freezing of already frozen device succeeds.
IOW it is a mess. We cannot change the behavior of the ioctl but we could
still provide an in-kernel interface to freeze_super() with the same
semantics as freeze_bdev() which might be easier to use by suspend - maybe
we could call it 'exclusive' (for the current freeze_super() semantics) and
'non-exclusive' (for the freeze_bdev() semantics) since this is very much
like O_EXCL open of block devices...

> 1) The new iterate supers stuff I added bail on the first error and return that
> error. If we kept the ioctl() interface error scheme we'd be erroring out
> if on suspend if userspace had already frozen a filesystem. Clearly that'd
> be silly so we need to distinguish between the automatic kernel freezing
> and the old userspace ioctl initiated interface, so that we can keep the
> old behaviour but allow in-kernel auto freeze on suspend to work properly.

This would work fine with the non-exclusive semantics I believe.

> 2) If we fail to suspend we need to then thaw up all filesystems. The order
> in which we try to freeze is in reverse order on the super_block list. If we
> fail though we iterate in proper order on the super_block list and thaw. If
> you had two filesystems this means that if a failure happened on freezing
> the first filesystem, we'd first thaw the other filesystem -- and because of
> 0) if we don't distinguish between the ioctl interface or auto freezing, we'd
> also fail on thaw'ing given the other superblock wouldn't have been frozen.
> 
> So we need to keep two separate approaches. The count stuff would not suffice
> to distinguish origin of source for freeze call.
> 
> Come to think of it, I think I forgot to avoid thaw if the freeze was ioctl
> initiated..
> 
> thaw_unlocked(bool cb_call)
> {
>   if (sb_frozen_by_cb(sb) && !cb_call)
>     return 0; /* skip as the user wanted to keep this fs frozen */
>   ...
> }
> 
> Even though the kernel auto call is new I think we need to keep ioctl initiated
> frozen filesystems frozen to not break old userspace assumptions.
> 
> So, keeping all this in mind, does a count method still suffice?

The count method would need a different error recovery method - i.e. if you
fail freezing filesystems somewhere in the middle of iteration through
superblock list, you'd need to iterate from that point on to the superblock
where you've started. This is somewhat more complicated than your approach
but it seems cleaner to me:

1) Function freezing all superblocks either succeeds and all superblocks
are frozen or fails and no superblocks are (additionally) frozen.

2) It is not that normal users + one special user (who owns the "flag" in
the superblock in form of a special freeze state) setup. We'd simply have
exclusive and non-exclusive users of superblock freezing and there can be
arbitrary numbers of them.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply index

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-29 23:23 [PATCH 00/11] fs: use freeze_fs on suspend/hibernate Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 01/11] fs: provide unlocked helper for freeze_super() Luis R. Rodriguez
2017-11-30 16:58   ` Jan Kara
2017-11-29 23:23 ` [PATCH 02/11] fs: provide unlocked helper thaw_super() Luis R. Rodriguez
2017-11-30 16:59   ` Jan Kara
2017-11-29 23:23 ` [PATCH 03/11] fs: add frozen sb state helpers Luis R. Rodriguez
2017-11-30 17:13   ` Jan Kara
2017-11-30 19:05     ` Luis R. Rodriguez
2017-12-01 11:47       ` Jan Kara [this message]
2017-12-01 21:13         ` Luis R. Rodriguez
2017-12-21 11:03           ` Jan Kara
2018-04-18  0:59             ` Luis R. Rodriguez
2018-04-18 10:12               ` Jan Kara
2018-04-20 18:49               ` Luis R. Rodriguez
2018-04-21 23:53                 ` Jan Kara
2018-04-22  1:22                   ` Luis R. Rodriguez
2018-04-22  2:53     ` Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 04/11] fs: distinguish between user initiated freeze and kernel initiated freeze Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 05/11] fs: add iterate_supers_excl() and iterate_supers_reverse_excl() Luis R. Rodriguez
2017-11-29 23:48   ` Rafael J. Wysocki
2017-11-30  0:22     ` Luis R. Rodriguez
2017-11-30  1:34     ` Dave Chinner
2017-11-30  1:40       ` Rafael J. Wysocki
2017-11-30 16:57   ` Jan Kara
2017-11-29 23:23 ` [PATCH 06/11] fs: freeze on suspend and thaw on resume Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 07/11] xfs: remove not needed freezing calls Luis R. Rodriguez
2017-11-30 16:21   ` Jan Kara
2017-11-30 20:32     ` Rafael J. Wysocki
2017-11-30 23:30       ` Dave Chinner
2017-11-30 23:40         ` Rafael J. Wysocki
2017-11-29 23:23 ` [PATCH 08/11] ext4: " Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 09/11] f2fs: " Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 10/11] nilfs2: " Luis R. Rodriguez
2017-11-29 23:23 ` [PATCH 11/11] jfs: " Luis R. Rodriguez
2017-11-30 16:36 ` [PATCH 00/11] fs: use freeze_fs on suspend/hibernate Yu Chen
2017-11-30 16:41   ` Jiri Kosina
2017-11-30 16:50     ` Yu Chen
2017-12-01 19:05     ` Jeff Layton
2017-12-01 21:51       ` Dave Chinner
2017-11-30 17:01 ` Bart Van Assche
2017-11-30 19:42   ` Luis R. Rodriguez
2017-11-30 20:53     ` Bart Van Assche
2017-11-30 21:03       ` Dave Chinner
2017-11-30 21:51 ` Pavel Machek
2017-12-01  0:44   ` Luis R. Rodriguez
2017-12-13  1:09 ` Rafael J. Wysocki
2017-12-19 16:50   ` Luis R. Rodriguez

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=20171201114724.GC8365@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=ONeukum@suse.com \
    --cc=bart.vanassche@wdc.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=jgross@suse.com \
    --cc=jikos@kernel.org \
    --cc=len.brown@intel.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=mcgrof@kernel.org \
    --cc=ming.lei@redhat.com \
    --cc=nborisov@suse.com \
    --cc=oleg.b.antonyan@gmail.com \
    --cc=oleksandr@natalenko.name \
    --cc=pavel@ucw.cz \
    --cc=rjw@rjwysocki.net \
    --cc=todd.e.brandt@linux.intel.com \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yu.chen.surf@gmail.com \
    /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

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git