Re: [git pull] mnt_devname queue

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Mar 17, 2011 at 03:28:47PM +0800, Xiaotian Feng wrote:
> Cc'ed author ...
> 
> > a) What the hell would you expect to happen if userland mounts it twice
> > and unmount the first one? ??pstore_sb = NULL, pstore_mnt = NULL, AFAICS.

BTW, you want
	* mount_single(), not mount_nodev()
	* simple_pin_fs()/mntput() around modifying that sucker from kernel
(held across both the file creation and writing to it)

> > b) pstore_writefile() - struct file on stack? ??Really? ??Again, in the
> > scenario above, what'll happen to you if pstore_mnt gets dropped and
> > freed in the middle of all that?
> >
> > c) in the same function:
> > + ?? ?? ?? memset(&f, '0', sizeof f);
> > Ahem...

Aside of a new meaning given to "zero that structure out", why the devil
are you doing it in such a convoluted way?  Note that you are using ramfs,
so the mapping is unevictable.  Simple kmalloc() + memcpy() and
simple_read_from_buffer() to implement ->read() would do nicely.

And for fsck sake, copy ramfs_get_inode() and trim it.  You are overriding
it for the single directory in there and you are only using it for regular
files otherwise.  With wrong ->i_op and ->i_fop *and* irrelevant messing with
->i_mapping.  Which leaves you with
	inode->i_ino = get_next_ino();
	inode_init_owner(inode, dir, mode);
	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
as useful part.

BTW, you want to make the contents visible before d_add().  You
*definitely* want to finish setting it up before unlocking the parent,
or you are asking for userland to come and unlink() it under you.

Incidentally, you are leaking ->i_private on umount.  You want ->evict_inode()
doing that kfree(), not ->unlink().  And possibly ->erase() as well, with
check for zero i_nlink around it (in ->evict_inode()).