lkml.org 
[lkml]   [2009]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: sget() misuse in nilfs
From
Hi,
On Sun, 3 May 2009 23:51:36 +0100, Al Viro wrote:
> OK, I give up; what _is_ get_sb/remount code supposed to implement?

Oh, these functions lack spec comments.

I first explain some specs. (I think part of them should be added on
the code, later)

The nilfs_get_sb() allocates a new super_block struct (sb) or assigns
an existing sb to the mount point through sget(). For newly allocated
sb it calls nilfs_fill_super() for initialization.

The following things are supposed here:

1) Every rw-mount on a device shares the same sb (as usual).

2) Every sb of snapshot is independent with that of rw-mount or other
snapshots if their checkpoint numbers differ. On the other hand,
two or more snapshots having the same checkpoint number share a sb
wherever possible.

3) Snapshots are mountable concurrently with a rw-mount, but a
ro-mount is not so because the ro-mount cannot follow changes
brought by the rw-mount.

4) Every sb on a device shares the same nilfs struct
(i.e. the_nilfs), which stores their common information.

The existing nilfs struct is acquired from an existing sb.
Otherwise, a new nilfs struct is allocated through alloc_nilfs()
function. The lifetime of the nilfs struct is managed with a
reference counter.

The nilfs_remount() function follows the change by given mount
options. This does not replace sb, so it involves the following
actions and confirmation:

a) A segment constructor (i.e. log writer of nilfs) is invoked on
ro->rw remount, and it is shut down for ro->rw remount.

b) ro->rw remount is possible only if there is no rw-mount on the
device and the checkpoint number of the ro-mount is latest.

c) Remounting snapshot to different checkpoints or rw-mount is not
allowed.

> Not to mention the reliability of down_read_trylock() in there (what
> happens if somebody tries to e.g. remount fs we are looking at the
> time? That's right, s_umount held exclusive, down_read_trulock()
> failing, fs instance skipped during the search), what is all that
> stuff trying to achieve?

It tries to find out the existing snapshot instance having a
checkpoint number which equals with the specified cp number (2).

Yeah, it may fail in the situation. I allow the miss at present
because it's harmless and rare though it wastes memory due to unwanted
duplication of the snapshot instance.

> What protects MS_RDONLY in sd->s_flags during these searches?

sd->s_flags is a local variable of the call sites of sget(), but, ugh,
s->s_flags is not protected. It may have problem...

BTW, MS_RDONLY in s->s_flags is also referred to in get_sb_bdev() and
do_remount_sb(). How is the protection achieved for these functions?

For example, ext3_remount() or ext3_handle_error() manipulates this
flag, and get_sb_bdev() or do_remount_sb() could see it for other
existing fs-instances.

> What makes parse_options() in remount straight into sb (with reversal
> if we'd done something bad) safe?

Well, it's not protected except lock_kernel() in the call sites.
I should add an argument to the parse_options() in order to forbid
destructive changes like ext3/4 does.

> Do we ever reassign snapshot_cno
> other than on sb creation and remounting between r/o and r/w?

We don't allow reassigning the snapshot_cno.
But, ugh, nilfs_remount() is rewriting it temporarily. It seems bad.
I'll fix it.

> Is there any reason why we free sbi early (== in put_super()) and
> not after kill_block_super() in ->kill_sb() of your own?

No, just I didn't think we have to delay it until ->kill_sb() because
bdev->bd_mount_sem and s->s_umount are protecting ->s_fs_info until
removing the sb from the type->fs_supers list and the super_blocks
list.

> That alone would make sbi stay with superblock for as long as it
> could be found by any means, killing the locking mess in your test
> callbacks.

If the advice turned out to be required or help for simplification,
I'll take it in. I need some time to think about it.

> Can SNAPSHOT even be there unless you have MS_RDONLY?

Yes, it can. Nilfs snapshots can exist concurrently with rw-mount.

> And what are the rules for exclusion in case of r/w mounts?

(answered in the first explanation)

> What, do we get -EBUSY on attempt to mount r/w something that is
> already mounted r/w (instead of simply sharing superblock, as other
> filesystems would do)?
>
> Or am I misreading that
> } else if (!(s->s_flags & MS_RDONLY)) {
> err = -EBUSY;
> }
> in there?

Hmm, this looks strange to me, too. Nilfs is sharing a r/w-mount as
described in (1). I'll confirm the -EBUSY case.

> Very confused Al...

Sorry. I recongize these are rather confusing.
The use of sget() and the test routines should be reduced to a minimum
if feasible.

Anyway, thanks for many questions and comments.

Regards,
Ryusuke Konishi


\
 
 \ /
  Last update: 2009-05-04 19:17    [W:0.273 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site