[git pull] mnt_devname queue

[git pull] mnt_devname queue

[Al Viro]

	Kills ->mnt_devname mangling in NFS, completes switch from ->get_sb()
to ->mount() (and kills the remnants of ->get_sb() helpers), makes /proc/mounts
and /proc/*/mountinfo show the right thing after mount --bind on NFS.  Trond
has ACKed the nfs side of things and suggested to push through the vfs tree,
so please pull from
git:git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ mnt_devname

Shortlog:
Al Viro (8):
      nfs: propagate devname to nfs{,4}_get_root()
      nfs: store devname at disconnected NFS roots
      nfs: make nfs_path() work without vfsmount
      nfs: nfs_do_{ref,sub}mount() superblock argument is redundant
      vfs: new superblock methods to override /proc/*/mount{s,info}
      nfs: stop mangling ->mnt_devname on NFS
      nfs: switch NFS from ->get_sb() to ->mount()
      vfs: bury ->get_sb()

Diffstat:
 Documentation/filesystems/Locking |    6 +-
 Documentation/filesystems/porting |    7 ++
 Documentation/filesystems/vfs.txt |   56 ++++++-----
 fs/namespace.c                    |   39 ++++++--
 fs/nfs/dir.c                      |   13 +++
 fs/nfs/getroot.c                  |   42 +++++++-
 fs/nfs/internal.h                 |   21 ++--
 fs/nfs/namespace.c                |   66 ++++++++-----
 fs/nfs/nfs4namespace.c            |   41 +++-----
 fs/nfs/super.c                    |  194 +++++++++++++++++++------------------
 fs/nfs/unlink.c                   |   20 ++++
 fs/super.c                        |   67 +------------
 include/linux/fs.h                |   16 +---
 13 files changed, 316 insertions(+), 272 deletions(-)

Re: [git pull] mnt_devname queue

[Xiaotian Feng]

On Thu, Mar 17, 2011 at 7:56 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>        Kills ->mnt_devname mangling in NFS, completes switch from ->get_sb()
> to ->mount() (and kills the remnants of ->get_sb() helpers), makes /proc/mounts
> and /proc/*/mountinfo show the right thing after mount --bind on NFS.  Trond
> has ACKed the nfs side of things and suggested to push through the vfs tree,
> so please pull from
> git:git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ mnt_devname
>
> Shortlog:
> Al Viro (8):
>      nfs: propagate devname to nfs{,4}_get_root()
>      nfs: store devname at disconnected NFS roots
>      nfs: make nfs_path() work without vfsmount
>      nfs: nfs_do_{ref,sub}mount() superblock argument is redundant
>      vfs: new superblock methods to override /proc/*/mount{s,info}
>      nfs: stop mangling ->mnt_devname on NFS
>      nfs: switch NFS from ->get_sb() to ->mount()
>      vfs: bury ->get_sb()

I guess we need also switch pstore from ->get_sb() to ->mount()

fs/pstore/inode.c:253: error: unknown field ‘get_sb’ specified in initializer
fs/pstore/inode.c:253: warning: initialization makes integer from
pointer without a cast
fs/pstore/inode.c:253: error: initializer element is not computable at load time
fs/pstore/inode.c:253: error: (near initialization for
‘pstore_fs_type.fs_flags’)

>
> Diffstat:
>  Documentation/filesystems/Locking |    6 +-
>  Documentation/filesystems/porting |    7 ++
>  Documentation/filesystems/vfs.txt |   56 ++++++-----
>  fs/namespace.c                    |   39 ++++++--
>  fs/nfs/dir.c                      |   13 +++
>  fs/nfs/getroot.c                  |   42 +++++++-
>  fs/nfs/internal.h                 |   21 ++--
>  fs/nfs/namespace.c                |   66 ++++++++-----
>  fs/nfs/nfs4namespace.c            |   41 +++-----
>  fs/nfs/super.c                    |  194 +++++++++++++++++++------------------
>  fs/nfs/unlink.c                   |   20 ++++
>  fs/super.c                        |   67 +------------
>  include/linux/fs.h                |   16 +---
>  13 files changed, 316 insertions(+), 272 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git pull] mnt_devname queue

[Al Viro]

On Thu, Mar 17, 2011 at 01:45:33PM +0800, Xiaotian Feng wrote:

> I guess we need also switch pstore from ->get_sb() to ->mount()
> 
> fs/pstore/inode.c:253: error: unknown field ???get_sb??? specified in initializer
> fs/pstore/inode.c:253: warning: initialization makes integer from
> pointer without a cast
> fs/pstore/inode.c:253: error: initializer element is not computable at load time
> fs/pstore/inode.c:253: error: (near initialization for
> ???pstore_fs_type.fs_flags???)

Yes.  I've just looked at that thing and I see several, er, issues.

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.

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

d)
+               if (pstore_is_mounted())
+                       pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
+                                     psinfo->buf, hsize + l1_cpy + l2_cpy,
+                                     CURRENT_TIME, psinfo->erase);

And what happens if it's unmounted between the check and use?  pstore_mkfile()
pretty much starts with dereferencing pstore_sb...

e) as the matter of fact, what happens if it's unmounted in the _middle_ of
pstore_mkfile()?

f) in general it's a lousy policy to tie that kind of difference in behaviour
to "somebody has it mounted someplace at the moment", even without the races
a-la (a)...

Re: [git pull] mnt_devname queue

[Xiaotian Feng]

On Thu, Mar 17, 2011 at 3:23 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Mar 17, 2011 at 01:45:33PM +0800, Xiaotian Feng wrote:
>
>> I guess we need also switch pstore from ->get_sb() to ->mount()
>>
>> fs/pstore/inode.c:253: error: unknown field ???get_sb??? specified in initializer
>> fs/pstore/inode.c:253: warning: initialization makes integer from
>> pointer without a cast
>> fs/pstore/inode.c:253: error: initializer element is not computable at load time
>> fs/pstore/inode.c:253: error: (near initialization for
>> ???pstore_fs_type.fs_flags???)
>
> Yes.  I've just looked at that thing and I see several, er, issues.
>

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.
>
> 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...
>
> d)
> +               if (pstore_is_mounted())
> +                       pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
> +                                     psinfo->buf, hsize + l1_cpy + l2_cpy,
> +                                     CURRENT_TIME, psinfo->erase);
>
> And what happens if it's unmounted between the check and use?  pstore_mkfile()
> pretty much starts with dereferencing pstore_sb...
>
> e) as the matter of fact, what happens if it's unmounted in the _middle_ of
> pstore_mkfile()?
>
> f) in general it's a lousy policy to tie that kind of difference in behaviour
> to "somebody has it mounted someplace at the moment", even without the races
> a-la (a)...
>

Re: [git pull] mnt_devname queue

[Al Viro]

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

Re: [git pull] mnt_devname queue

[Tony Luck]

On Thu, Mar 17, 2011 at 3:44 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Mar 17, 2011 at 03:28:47PM +0800, Xiaotian Feng wrote:

Thanks for all the tips - I'll work on fixing (re-writing :-) pstore

-Tony

Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Tony Luck]

Al,

I haven't got all your suggestions implemented yet - but thought I'd do
a quick direction check to see if I understood your e-mail.

Fixes below:
1) Change from ->get_sb() to ->mount()
2) Use mount_single() instead of mount_nodev()
3) Pulled in ramfs_get_inode() & trimmed to what I need for pstore
4) Drop the ugly pstore_writefile() Just save data using kmalloc() and
   provide a pstore_file_read() that uses simple_read_from_buffer().

Still todo:
1) Handle multiple mounts
2) Use simple_pin_fs/mntput to prevent races
3) Move cleanup from unlink to evict_inode
4) Stop it from saving dmesg on a clean "reboot" [!! Not sure when it started doing this !!]


-Tony

---

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 549d245..7d848b1 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -40,9 +40,29 @@
 struct pstore_private {
 	u64	id;
 	int	(*erase)(u64);
+	ssize_t	size;
+	char	data[];
 };
 
-#define pstore_get_inode ramfs_get_inode
+static int pstore_file_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
+						size_t count, loff_t *ppos)
+{
+	struct pstore_private *ps = file->private_data;
+
+	return simple_read_from_buffer(userbuf, count, ppos, ps->data, ps->size);
+}
+
+static const struct file_operations pstore_file_operations = {
+	.open	= pstore_file_open,
+	.read	= pstore_file_read,
+	.llseek	= default_llseek,
+};
 
 /*
  * When a file is unlinked from our file system we call the
@@ -63,6 +83,30 @@ static const struct inode_operations pstore_dir_inode_operations = {
 	.unlink		= pstore_unlink,
 };
 
+static struct inode *pstore_get_inode(struct super_block *sb,
+					const struct inode *dir, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+
+	if (inode) {
+		inode->i_ino = get_next_ino();
+		inode->i_uid = inode->i_gid = 0;
+		inode->i_mode = mode;
+		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		switch (mode & S_IFMT) {
+		case S_IFREG:
+			inode->i_fop = &pstore_file_operations;
+			break;
+		case S_IFDIR:
+			inode->i_op = &pstore_dir_inode_operations;
+			inode->i_fop = &simple_dir_operations;
+			inc_nlink(inode);
+			break;
+		}
+	}
+	return inode;
+}
+
 static const struct super_operations pstore_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
@@ -70,37 +114,10 @@ static const struct super_operations pstore_ops = {
 };
 
 static struct super_block *pstore_sb;
-static struct vfsmount *pstore_mnt;
 
 int pstore_is_mounted(void)
 {
-	return pstore_mnt != NULL;
-}
-
-/*
- * Set up a file structure as if we had opened this file and
- * write our data to it.
- */
-static int pstore_writefile(struct inode *inode, struct dentry *dentry,
-	char *data, size_t size)
-{
-	struct file f;
-	ssize_t n;
-	mm_segment_t old_fs = get_fs();
-
-	memset(&f, '0', sizeof f);
-	f.f_mapping = inode->i_mapping;
-	f.f_path.dentry = dentry;
-	f.f_path.mnt = pstore_mnt;
-	f.f_pos = 0;
-	f.f_op = inode->i_fop;
-	set_fs(KERNEL_DS);
-	n = do_sync_write(&f, data, size, &f.f_pos);
-	set_fs(old_fs);
-
-	fsnotify_modify(&f);
-
-	return n == size;
+	return pstore_sb != NULL;
 }
 
 /*
@@ -123,8 +140,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
 	if (!inode)
 		goto fail;
-	inode->i_uid = inode->i_gid = 0;
-	private = kmalloc(sizeof *private, GFP_KERNEL);
+	private = kmalloc(sizeof *private + size, GFP_KERNEL);
 	if (!private)
 		goto fail_alloc;
 	private->id = id;
@@ -156,8 +172,8 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 
 	mutex_unlock(&root->d_inode->i_mutex);
 
-	if (!pstore_writefile(inode, dentry, data, size))
-		goto fail_write;
+	memcpy(private->data, data, size);
+	inode->i_size = private->size = size;
 
 	inode->i_private = private;
 
@@ -166,15 +182,6 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 
 	return 0;
 
-fail_write:
-	kfree(private);
-	inode->i_nlink--;
-	mutex_lock(&root->d_inode->i_mutex);
-	d_delete(dentry);
-	dput(dentry);
-	mutex_unlock(&root->d_inode->i_mutex);
-	goto fail;
-
 fail_lockedalloc:
 	mutex_unlock(&root->d_inode->i_mutex);
 	kfree(private);
@@ -225,32 +232,27 @@ fail:
 	return err;
 }
 
-static int pstore_get_sb(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
+static struct dentry *pstore_mount(struct file_system_type *fs_type,
+	int flags, const char *dev_name, void *data)
 {
 	struct dentry *root;
 
-	root = mount_nodev(fs_type, flags, data, pstore_fill_super);
+	root = mount_single(fs_type, flags, data, pstore_fill_super);
 	if (IS_ERR(root))
-		return -ENOMEM;
-
-	mnt->mnt_root = root;
-	mnt->mnt_sb = root->d_sb;
-	pstore_mnt = mnt;
+		return root;
 
-	return 0;
+	return root;
 }
 
 static void pstore_kill_sb(struct super_block *sb)
 {
 	kill_litter_super(sb);
 	pstore_sb = NULL;
-	pstore_mnt = NULL;
 }
 
 static struct file_system_type pstore_fs_type = {
 	.name		= "pstore",
-	.get_sb		= pstore_get_sb,
+	.mount		= pstore_mount,
 	.kill_sb	= pstore_kill_sb,
 };
 

Re: Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Al Viro]

On Thu, Mar 17, 2011 at 02:35:42PM -0700, Tony Luck wrote:
>  	mutex_unlock(&root->d_inode->i_mutex);
>  
> -	if (!pstore_writefile(inode, dentry, data, size))
> -		goto fail_write;
> +	memcpy(private->data, data, size);
> +	inode->i_size = private->size = size;
>  
>  	inode->i_private = private;

Better move that before d_add()

>  	if (IS_ERR(root))
> -		return -ENOMEM;
> -
> -	mnt->mnt_root = root;
> -	mnt->mnt_sb = root->d_sb;
> -	pstore_mnt = mnt;
> +		return root;
>  
> -	return 0;
> +	return root;

*blink*
	if (IS_ERR(root))
		return root;
	return root;

... or am I misreading the patch?

Re: Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Tony Luck]

On Thu, Mar 17, 2011 at 3:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> +     memcpy(private->data, data, size);
>> +     inode->i_size = private->size = size;
>>
>>       inode->i_private = private;
>
> Better move that before d_add()

Will do.

> *blink*
>        if (IS_ERR(root))
>                return root;
>        return root;
>
> ... or am I misreading the patch?

Your patch reading skills are fine ... the code deserves more than a *blink*.
Will fix.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Al Viro]

On Thu, Mar 17, 2011 at 03:48:13PM -0700, Tony Luck wrote:
> On Thu, Mar 17, 2011 at 3:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> + ? ? memcpy(private->data, data, size);
> >> + ? ? inode->i_size = private->size = size;
> >>
> >> ? ? ? inode->i_private = private;
> >
> > Better move that before d_add()
> 
> Will do.
> 
> > *blink*
> > ? ? ? ?if (IS_ERR(root))
> > ? ? ? ? ? ? ? ?return root;
> > ? ? ? ?return root;
> >
> > ... or am I misreading the patch?
> 
> Your patch reading skills are fine ... the code deserves more than a *blink*.
> Will fix.

Send the updated patch my way, please.

Re: Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Tony Luck]

> Send the updated patch my way, please.

Here it is.

-Tony

---

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 549d245..0834223 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -40,9 +40,29 @@
 struct pstore_private {
 	u64	id;
 	int	(*erase)(u64);
+	ssize_t	size;
+	char	data[];
 };
 
-#define pstore_get_inode ramfs_get_inode
+static int pstore_file_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
+						size_t count, loff_t *ppos)
+{
+	struct pstore_private *ps = file->private_data;
+
+	return simple_read_from_buffer(userbuf, count, ppos, ps->data, ps->size);
+}
+
+static const struct file_operations pstore_file_operations = {
+	.open	= pstore_file_open,
+	.read	= pstore_file_read,
+	.llseek	= default_llseek,
+};
 
 /*
  * When a file is unlinked from our file system we call the
@@ -63,6 +83,30 @@ static const struct inode_operations pstore_dir_inode_operations = {
 	.unlink		= pstore_unlink,
 };
 
+static struct inode *pstore_get_inode(struct super_block *sb,
+					const struct inode *dir, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+
+	if (inode) {
+		inode->i_ino = get_next_ino();
+		inode->i_uid = inode->i_gid = 0;
+		inode->i_mode = mode;
+		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		switch (mode & S_IFMT) {
+		case S_IFREG:
+			inode->i_fop = &pstore_file_operations;
+			break;
+		case S_IFDIR:
+			inode->i_op = &pstore_dir_inode_operations;
+			inode->i_fop = &simple_dir_operations;
+			inc_nlink(inode);
+			break;
+		}
+	}
+	return inode;
+}
+
 static const struct super_operations pstore_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
@@ -70,37 +114,10 @@ static const struct super_operations pstore_ops = {
 };
 
 static struct super_block *pstore_sb;
-static struct vfsmount *pstore_mnt;
 
 int pstore_is_mounted(void)
 {
-	return pstore_mnt != NULL;
-}
-
-/*
- * Set up a file structure as if we had opened this file and
- * write our data to it.
- */
-static int pstore_writefile(struct inode *inode, struct dentry *dentry,
-	char *data, size_t size)
-{
-	struct file f;
-	ssize_t n;
-	mm_segment_t old_fs = get_fs();
-
-	memset(&f, '0', sizeof f);
-	f.f_mapping = inode->i_mapping;
-	f.f_path.dentry = dentry;
-	f.f_path.mnt = pstore_mnt;
-	f.f_pos = 0;
-	f.f_op = inode->i_fop;
-	set_fs(KERNEL_DS);
-	n = do_sync_write(&f, data, size, &f.f_pos);
-	set_fs(old_fs);
-
-	fsnotify_modify(&f);
-
-	return n == size;
+	return pstore_sb != NULL;
 }
 
 /*
@@ -123,8 +140,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	inode = pstore_get_inode(pstore_sb, root->d_inode, S_IFREG | 0444, 0);
 	if (!inode)
 		goto fail;
-	inode->i_uid = inode->i_gid = 0;
-	private = kmalloc(sizeof *private, GFP_KERNEL);
+	private = kmalloc(sizeof *private + size, GFP_KERNEL);
 	if (!private)
 		goto fail_alloc;
 	private->id = id;
@@ -152,28 +168,19 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	if (IS_ERR(dentry))
 		goto fail_lockedalloc;
 
-	d_add(dentry, inode);
-
-	mutex_unlock(&root->d_inode->i_mutex);
-
-	if (!pstore_writefile(inode, dentry, data, size))
-		goto fail_write;
+	memcpy(private->data, data, size);
+	inode->i_size = private->size = size;
 
 	inode->i_private = private;
 
 	if (time.tv_sec)
 		inode->i_mtime = inode->i_ctime = time;
 
-	return 0;
+	d_add(dentry, inode);
 
-fail_write:
-	kfree(private);
-	inode->i_nlink--;
-	mutex_lock(&root->d_inode->i_mutex);
-	d_delete(dentry);
-	dput(dentry);
 	mutex_unlock(&root->d_inode->i_mutex);
-	goto fail;
+
+	return 0;
 
 fail_lockedalloc:
 	mutex_unlock(&root->d_inode->i_mutex);
@@ -225,32 +232,21 @@ fail:
 	return err;
 }
 
-static int pstore_get_sb(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
+static struct dentry *pstore_mount(struct file_system_type *fs_type,
+	int flags, const char *dev_name, void *data)
 {
-	struct dentry *root;
-
-	root = mount_nodev(fs_type, flags, data, pstore_fill_super);
-	if (IS_ERR(root))
-		return -ENOMEM;
-
-	mnt->mnt_root = root;
-	mnt->mnt_sb = root->d_sb;
-	pstore_mnt = mnt;
-
-	return 0;
+	return mount_single(fs_type, flags, data, pstore_fill_super);
 }
 
 static void pstore_kill_sb(struct super_block *sb)
 {
 	kill_litter_super(sb);
 	pstore_sb = NULL;
-	pstore_mnt = NULL;
 }
 
 static struct file_system_type pstore_fs_type = {
 	.name		= "pstore",
-	.get_sb		= pstore_get_sb,
+	.mount		= pstore_mount,
 	.kill_sb	= pstore_kill_sb,
 };
 

Re: Some fixes for pstore (Was Re: [git pull] mnt_devname queue)

[Al Viro]

On Thu, Mar 17, 2011 at 04:29:15PM -0700, Tony Luck wrote:
> > Send the updated patch my way, please.
> 
> Here it is.

Applied.  I'll add simple_pin_fs()/mntput() to deal with the races as a
separate commit and put that into tonight's pull request.

pstore: fix leaking ->i_private

[Luck, Tony]

From: Tony Luck <tony.luck@intel.com>

Move kfree() of i_private out of ->unlink() and into ->evict_inode()

Signed-off-by: Tony Luck <tony.luck@intel.com>

---

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

I think this does what I want.

I only want to ->erase() the persistant store if there has been an unlink(2)
on the file.  It looks to be safe and right to do this in ->unlink()

I don't think I need any checks on i_nlink - pstore filesystem won't let you
make extra links to a file.

end_writeback(inode) looks to be somewhat overkill, all I need is to set I_CLEAR
bit in ->i_state. But nobody else does this by hand, and the extra actions in
end_writeback() look to be harmless.

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0834223..e8b0c08 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -73,11 +73,16 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 	struct pstore_private *p = dentry->d_inode->i_private;
 
 	p->erase(p->id);
-	kfree(p);
 
 	return simple_unlink(dir, dentry);
 }
 
+static void pstore_evict_inode(struct inode *inode)
+{
+	kfree(inode->i_private);
+	end_writeback(inode);
+}
+
 static const struct inode_operations pstore_dir_inode_operations = {
 	.lookup		= simple_lookup,
 	.unlink		= pstore_unlink,
@@ -110,6 +115,7 @@ static struct inode *pstore_get_inode(struct super_block *sb,
 static const struct super_operations pstore_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
+	.evict_inode	= pstore_evict_inode,
 	.show_options	= generic_show_options,
 };
 

Re: pstore: fix leaking ->i_private

[Christoph Hellwig]

Another comment on pstore:

 - the kmsg_bytes attribute really should be a mount option.  no need
   have this in a completely different namespace, and require all the
   kobject mess around it.

Re: pstore: fix leaking ->i_private

[Tony Luck]

On Fri, Mar 18, 2011 at 11:49 AM, Christoph Hellwig <hch@infradead.org> wrote:
>  - the kmsg_bytes attribute really should be a mount option.  no need
>   have this in a completely different namespace, and require all the
>   kobject mess around it.

Sounds cleaner - I'll take a look.

Thanks

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pstore: fix leaking ->i_private

[Al Viro]

On Fri, Mar 18, 2011 at 11:44:48AM -0700, Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> Move kfree() of i_private out of ->unlink() and into ->evict_inode()
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>

Applied (with one trivial change - in your case it doesn't matter, but
people do copy from weird places and conceptually you want to get VFS
end any async activity on inode before starting to tear it apart, so
end_writeback() should normally go first).

pstore: use mount option instead sysfs to tweak kmsg_bytes

[Luck, Tony]

/sys/fs is a somewhat strange way to tweak what could more
obviously be tuned with a mount option.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>

---

Christoph Hellwig wrote:
>  - the kmsg_bytes attribute really should be a mount option.  no need
>    have this in a completely different namespace, and require all the
>    kobject mess around it.

Al: If this looks OK, will you run it through your vfs tree please?

diff --git a/Documentation/ABI/testing/pstore b/Documentation/ABI/testing/pstore
index f1fb2a0..8232d2f 100644
--- a/Documentation/ABI/testing/pstore
+++ b/Documentation/ABI/testing/pstore
@@ -1,6 +1,6 @@
 Where:		/dev/pstore/...
-Date:		January 2011
-Kernel Version: 2.6.38
+Date:		March 2011
+Kernel Version: 2.6.39
 Contact:	tony.luck@intel.com
 Description:	Generic interface to platform dependent persistent storage.
 
@@ -11,7 +11,7 @@ Description:	Generic interface to platform dependent persistent storage.
 		of the console log is captured, but other interesting
 		data can also be saved.
 
-		# mount -t pstore - /dev/pstore
+		# mount -t pstore -o kmsg_bytes=8000 - /dev/pstore
 
 		$ ls -l /dev/pstore
 		total 0
@@ -33,3 +33,9 @@ Description:	Generic interface to platform dependent persistent storage.
 		will be saved elsewhere and erased from persistent store
 		soon after boot to free up space ready for the next
 		catastrophe.
+
+		The 'kmsg_bytes' mount option changes the target amount of
+		data saved on each oops/panic. Pstore saves (possibly
+		multiple) files based on the record size of the underlying
+		persistent storage until at least this amount is reached.
+		Default is 10 Kbytes.
diff --git a/Documentation/ABI/testing/sysfs-fs-pstore b/Documentation/ABI/testing/sysfs-fs-pstore
deleted file mode 100644
index 8e659d8..0000000
--- a/Documentation/ABI/testing/sysfs-fs-pstore
+++ /dev/null
@@ -1,7 +0,0 @@
-What:		/sys/fs/pstore/kmsg_bytes
-Date:		January 2011
-Kernel Version: 2.6.38
-Contact:	"Tony Luck" <tony.luck@intel.com>
-Description:
-		Controls amount of console log that will be saved
-		to persistent store on oops/panic.
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index f777f29..52cb6d4 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -27,6 +27,7 @@
 #include <linux/string.h>
 #include <linux/mount.h>
 #include <linux/ramfs.h>
+#include <linux/parser.h>
 #include <linux/sched.h>
 #include <linux/magic.h>
 #include <linux/pstore.h>
@@ -112,10 +113,52 @@ static struct inode *pstore_get_inode(struct super_block *sb,
 	return inode;
 }
 
+enum {
+	Opt_kmsg_bytes, Opt_err
+};
+
+static const match_table_t tokens = {
+	{Opt_kmsg_bytes, "kmsg_bytes=%u"},
+	{Opt_err, NULL}
+};
+
+static void parse_options(char *options)
+{
+	char		*p;
+	substring_t	args[MAX_OPT_ARGS];
+	int		option;
+
+	if (!options)
+		return;
+
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
+
+		if (!*p)
+			continue;
+
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_kmsg_bytes:
+			if (!match_int(&args[0], &option))
+				pstore_set_kmsg_bytes(option);
+			break;
+		}
+	}
+}
+
+static int pstore_remount(struct super_block *sb, int *flags, char *data)
+{
+	parse_options(data);
+
+	return 0;
+}
+
 static const struct super_operations pstore_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
 	.evict_inode	= pstore_evict_inode,
+	.remount_fs	= pstore_remount,
 	.show_options	= generic_show_options,
 };
 
@@ -215,6 +258,8 @@ int pstore_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op		= &pstore_ops;
 	sb->s_time_gran		= 1;
 
+	parse_options(data);
+
 	inode = pstore_get_inode(sb, NULL, S_IFDIR | 0755, 0);
 	if (!inode) {
 		err = -ENOMEM;
@@ -258,28 +303,7 @@ static struct file_system_type pstore_fs_type = {
 
 static int __init init_pstore_fs(void)
 {
-	int rc = 0;
-	struct kobject *pstorefs_kobj;
-
-	pstorefs_kobj = kobject_create_and_add("pstore", fs_kobj);
-	if (!pstorefs_kobj) {
-		rc = -ENOMEM;
-		goto done;
-	}
-
-	rc = sysfs_create_file(pstorefs_kobj, &pstore_kmsg_bytes_attr.attr);
-	if (rc)
-		goto done1;
-
-	rc = register_filesystem(&pstore_fs_type);
-	if (rc == 0)
-		goto done;
-
-	sysfs_remove_file(pstorefs_kobj, &pstore_kmsg_bytes_attr.attr);
-done1:
-	kobject_put(pstorefs_kobj);
-done:
-	return rc;
+	return register_filesystem(&pstore_fs_type);
 }
 module_init(init_pstore_fs)
 
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 76c26d2..8c9f23e 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -1,7 +1,6 @@
+extern void	pstore_set_kmsg_bytes(int);
 extern void	pstore_get_records(void);
 extern int	pstore_mkfile(enum pstore_type_id, char *psname, u64 id,
 			      char *data, size_t size,
 			      struct timespec time, int (*erase)(u64));
 extern int	pstore_is_mounted(void);
-
-extern struct kobj_attribute pstore_kmsg_bytes_attr;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 705fdf8..ce9ad84 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -37,24 +37,14 @@
 static DEFINE_SPINLOCK(pstore_lock);
 static struct pstore_info *psinfo;
 
-/* How much of the console log to snapshot. /sys/fs/pstore/kmsg_bytes */
+/* How much of the console log to snapshot */
 static unsigned long kmsg_bytes = 10240;
 
-static ssize_t b_show(struct kobject *kobj,
-		      struct kobj_attribute *attr, char *buf)
+void pstore_set_kmsg_bytes(int bytes)
 {
-	return snprintf(buf, PAGE_SIZE, "%lu\n", kmsg_bytes);
+	kmsg_bytes = bytes;
 }
 
-static ssize_t b_store(struct kobject *kobj, struct kobj_attribute *attr,
-		       const char *buf, size_t count)
-{
-	return (sscanf(buf, "%lu", &kmsg_bytes) > 0) ? count : 0;
-}
-
-struct kobj_attribute pstore_kmsg_bytes_attr =
-	__ATTR(kmsg_bytes, S_IRUGO | S_IWUSR, b_show, b_store);
-
 /* Tag each group of saved records with a sequence number */
 static int	oopscount;