On Wed, Jan 11, 2012 at 03:53:24PM -0600, Eric Sandeen wrote:
> On 1/6/12 8:09 AM, Jan Kara wrote:
> > Hello,
> >
> > I was looking at what causes filesystem to have dirty data after it is
> > frozen. After some thought I realized freezing code is inherently racy and
> > all filesystems (ext3, ext4, xfs) can have dirty data on frozen filesystem.
> >
> > The race is basically following:
> > Task 1 Task 2
> > freeze_super() __generic_file_aio_write()
> > ... vfs_check_frozen(sb, SB_FREEZE_WRITE)
> > sb->s_frozen = SB_FREEZE_WRITE;
> > sync_filesystem(sb);
> > do the write
> > /* Here we create dirty data
> > * which is left on frozen fs */
> > sb->s_frozen = SB_FREEZE_TRANS;
> > ...
> > ->freeze_fs()
> >
> > The problem is that you can never make checking for frozen filesystem
> > race-free with the current s_frozen scheme - the filesystem can always be
> > frozen the instant after you check for it and you end up creating dirty
> > data on frozen filesystem.
> >
> > The question is what to do with this problem. I outline the possibilities
> > that come to my mind below:
> > 1) Ignore the problem - depending on the exact fs details this could lead to
> > fs snapshot being corrupted, also flusher thread can hang on the frozen
> > filesystem (e.g. because of sync(1)) creating all sorts of secondary
> > issues. So I don't think this is really an option.
> > 2) Have a rwlock in the superblock that is held for writing while
> > filesystem freezing is in progress and held for reading by the filesystem
> > while a transaction is running except for transactions that are required
> > to do writeback. This is kind of ugly but at least for ext3/4 relatively
> > easy to implement.
>
> This is as far as I had gotten while independently thinking about it ;)
>
> But talking with dchinner, he had concerns about the scalability of any
> rwlock, and I think we (ok, mostly Dave) came up with another idea.
>
> What if we had 2 counters in the superblock, one for the equivalent of
> SB_FREEZE_WRITE and one for SB_FREEZE_TRANS. These would use similar
> infrastructure to mnt_want_write et al.
>
> Everywhere we currently vfs_check_frozen() we'd have a better-named function
> which increments the counter, then checks the freeze level. If we are
> being frozen, we drop the counter & wait. If not frozen, we continue;
> like this pseudo-code:
>
> void super_freeze_wait(sb, level) {
> while (1) {
> level_ref++;
> if (!frozen(sb, level))
> return; /* not freezing */
> level_ref--;
> wait_unfrozen(sb, level);
> }
> }
>
> There would also be new code to drop the counter when the dirtying is complete.
>
> The freezing functions then just have to wait until the counters hit zero
> before they can consider themselves done, and freezing is complete. That way if
> someone sneaks in while the freeze level is being set, they have already
> notified their intent, and freeze can wait for it anyway before returning.
Just to clarify, freeze_super would do:
sb->s_frozen = SB_FREEZE_WRITE;
smp_wmb();
while (sb->s_active_write_cnt > 0)
wait;
/* no new or existing dirtying writers now, safe to sync */
sync_filesystem(sb);
sb->s_frozen = SB_FREEZE_TRANS;
smp_wmb();
while (sb->s_active_trans_cnt > 0)
wait;
/* no new or existing transactions in progress now, so freeze */
sb->s_op->freeze_fs(sb);
The counter implemetations will need to scale (e.g. per-cpu
counters) and we could probably use a generic waitqueue, but I think
this can all be implemented at the superblock level and we only need
to call the inc/dec helper functions in the correct places to make
it all work.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
