linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Omar Sandoval <osandov@osandov.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, kernel-team@fb.com,
	linux-fsdevel@vger.kernel.org, Tejun Heo <tj@kernel.org>
Subject: Re: [RFC PATCH v4 4/6] Btrfs: prevent ioctls from interfering with a swap file
Date: Tue, 21 Aug 2018 01:46:07 -0700	[thread overview]
Message-ID: <20180821084607.GA2119@vader> (raw)
In-Reply-To: <20180525161042.GR6649@twin.jikos.cz>

On Fri, May 25, 2018 at 06:10:43PM +0200, David Sterba wrote:
> On Fri, May 25, 2018 at 09:00:58AM -0700, Omar Sandoval wrote:
> > On Fri, May 25, 2018 at 04:50:55PM +0200, David Sterba wrote:
> > > On Thu, May 24, 2018 at 02:41:28PM -0700, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > > 
> > > > When a swap file is active, we must make sure that the extents of the
> > > > file are not moved and that they don't become shared. That means that
> > > > the following are not safe:
> > > > 
> > > > - chattr +c (enable compression)
> > > > - reflink
> > > > - dedupe
> > > > - snapshot
> > > > - defrag
> > > > - balance
> > > > - device remove/replace/resize
> > > > 
> > > > Don't allow those to happen on an active swap file. Balance and device
> > > > remove/replace/resize in particular are disallowed entirely; in the
> > > > future, we can relax this so that relocation skips/errors out only on
> > > > chunks containing an active swap file.
> > > 
> > > Hm, disabling the entire balance is too intrusive. It's clear that the
> > > swapfile causes a lot of trouble when it goes against the dynamic
> > > capabilities of btrfs (relocation and the functionality that builds on
> > > it).
> > > 
> > > Skipping the swapfile extents should be implemented at minimum.
> > 
> > Sure thing, this should definitely be possible. For balance, we can skip
> > them; for resize or delete, it of course has to fail if it encounters
> > swap extents. I'll take a stab at it.
> 
> We can detect if there's an active swap file on the filesystem before
> shrink, delete or replace is started so the user is not surprised if it
> fails in the end, or not start the operations at all and give some hints
> what to do.

I looked at this some more, it's not pretty... Basically, we need to

- Add a counter of active swap extents to struct btrfs_block_group_cache
- At activate time, map the file extent to a block group and increment the counter
- At relocation time, check the counter, and either error out or skip as
  appropriate
- At deactivate time, decrement all of the block group counters. Easier
  said than done because the file could in theory have new extents
  allocated beyond EOF

The last point is the tricky one. The straightforward way to implement
deactivate would be to walk all of the extents of the file in the same
way we did for activate, but the extents may have changed. So, we have
to remember which extents were activated (or get that information from
the generic swap code somehow). This seems fragile.

Does anyone see a better approach? Is it worth the trouble?

  reply	other threads:[~2018-08-21 12:05 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-24 21:41 [RFC PATCH v4 0/6] Btrfs: implement swap file support Omar Sandoval
2018-05-24 21:41 ` [RFC PATCH v4 1/6] mm: split SWP_FILE into SWP_ACTIVATED and SWP_FS Omar Sandoval
2018-05-25  9:11   ` Nikolay Borisov
2018-05-24 21:41 ` [RFC PATCH v4 2/6] vfs: update swap_{,de}activate documentation Omar Sandoval
2018-05-25  9:15   ` Nikolay Borisov
2018-05-24 21:41 ` [RFC PATCH v4 3/6] Btrfs: push EXCL_OP set into btrfs_rm_device() Omar Sandoval
2018-05-25  9:19   ` Nikolay Borisov
2018-05-28 13:29   ` David Sterba
2018-05-24 21:41 ` [RFC PATCH v4 4/6] Btrfs: prevent ioctls from interfering with a swap file Omar Sandoval
2018-05-25 14:50   ` David Sterba
2018-05-25 16:00     ` Omar Sandoval
2018-05-25 16:10       ` David Sterba
2018-08-21  8:46         ` Omar Sandoval [this message]
2018-05-24 21:41 ` [RFC PATCH v4 5/6] Btrfs: rename get_chunk_map() and make it non-static Omar Sandoval
2018-05-25  9:21   ` Nikolay Borisov
2018-05-25 16:02     ` Omar Sandoval
2018-05-24 21:41 ` [RFC PATCH v4 6/6] Btrfs: support swap files Omar Sandoval
2018-05-25 10:07   ` Nikolay Borisov
2018-05-25 16:16     ` Omar Sandoval
2018-05-28 13:46   ` David Sterba

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=20180821084607.GA2119@vader \
    --to=osandov@osandov.com \
    --cc=dsterba@suse.cz \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=tj@kernel.org \
    --subject='Re: [RFC PATCH v4 4/6] Btrfs: prevent ioctls from interfering with a swap file' \
    /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

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).