Skip to contentSkip to conversation

libfs,xenfs: replace /proc/xen/xenbus with a symlink

PATCH Superseded David Vrabel 3 participants 9 messages Jun 28, 2016 – Aug 23, 2016

Subsystems (2)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover libfs,xenfs: replace /proc/xen/xenbus with a symlink

Using /proc/xen/xenbus can cause deadlocks on the atomic file position mutex since this file should behave like a character device and not a regular file. This is easiest to achive by making it a symlink to the existing /dev/xen/xenbus device.

This requires extending simple_fill_super() to add symlinks as well as regular files.

Changes in v3:
- Rebased on v4.7-rc5.

David

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#1 lore raw

PATCH v3 1/2 libfs: allow simple_fill_super() to add symlinks

simple_fill_super() will add symlinks if an entry has mode & S_IFLNK. The target is provided in the new "link" field.

---
v2:
- simplified.

---
 fs/libfs.c         | 15 +++++++++++++--
 include/linux/fs.h |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)
fs/libfs.c +13 -2
index cedeacb..bbf2d82 100644
@@ -512,9 +512,20 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 			dput(dentry);
 			goto out;
 		}
-		inode->i_mode = S_IFREG | files->mode;
+		if (files->mode & S_IFLNK) {
+			inode->i_mode = files->mode;
+			inode->i_op = &simple_symlink_inode_operations;
+			inode->i_link = kstrdup(files->link, GFP_KERNEL);
+			if (!inode->i_link) {
+				iput(inode);
+				dput(dentry);
+				goto out;
+			}
+		} else {
+			inode->i_mode = S_IFREG | files->mode;
+			inode->i_fop = files->ops;
+		}
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
-		inode->i_fop = files->ops;
 		inode->i_ino = i;
 		d_add(dentry, inode);
 	}
include/linux/fs.h +1 -1
index dd28814..20c54a2 100644
@@ -2953,7 +2953,7 @@ extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 extern void make_empty_dir_inode(struct inode *inode);
 extern bool is_empty_dir_inode(struct inode *inode);
-struct tree_descr { char *name; const struct file_operations *ops; int mode; };
+struct tree_descr { char *name; const struct file_operations *ops; int mode; char *link; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
 extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
Signature · 1 line
2.1.4
#2 lore raw

Re: [Xen-devel] [PATCHv3 1/2] libfs: allow simple_fill_super() to add symlinks

Quoted 2 lines · David Vrabel

simple_fill_super() will add symlinks if an entry has mode & S_IFLNK. The target is provided in the new "link" field.

Can I get an ack for this, please? So it can go into 4.8 via the Xen tree.

David

Quoted 51 lines

---
v2:
- simplified.

---
 fs/libfs.c         | 15 +++++++++++++--
 include/linux/fs.h |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)
fs/libfs.c +13 -2
index cedeacb..bbf2d82 100644
@@ -512,9 +512,20 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 			dput(dentry);
 			goto out;
 		}
-		inode->i_mode = S_IFREG | files->mode;
+		if (files->mode & S_IFLNK) {
+			inode->i_mode = files->mode;
+			inode->i_op = &simple_symlink_inode_operations;
+			inode->i_link = kstrdup(files->link, GFP_KERNEL);
+			if (!inode->i_link) {
+				iput(inode);
+				dput(dentry);
+				goto out;
+			}
+		} else {
+			inode->i_mode = S_IFREG | files->mode;
+			inode->i_fop = files->ops;
+		}
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
-		inode->i_fop = files->ops;
 		inode->i_ino = i;
 		d_add(dentry, inode);
 	}
include/linux/fs.h +1 -1
index dd28814..20c54a2 100644
@@ -2953,7 +2953,7 @@ extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 extern void make_empty_dir_inode(struct inode *inode);
 extern bool is_empty_dir_inode(struct inode *inode);
-struct tree_descr { char *name; const struct file_operations *ops; int mode; };
+struct tree_descr { char *name; const struct file_operations *ops; int mode; char *link; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
 extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
#3 lore raw

Re: [PATCHv3 1/2] libfs: allow simple_fill_super() to add symlinks

In reply to David Vrabel
Quoted 2 lines · David Vrabel

simple_fill_super() will add symlinks if an entry has mode & S_IFLNK. The target is provided in the new "link" field.

Can I get an ack for this, please? So it can go into 4.8 via the Xen tree.

David

Quoted 43 lines

---
v2:
- simplified.

---
 fs/libfs.c         | 15 +++++++++++++--
 include/linux/fs.h |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)
fs/libfs.c +13 -2
index cedeacb..bbf2d82 100644
@@ -512,9 +512,20 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
 			dput(dentry);
 			goto out;
 		}
-		inode->i_mode = S_IFREG | files->mode;
+		if (files->mode & S_IFLNK) {
+			inode->i_mode = files->mode;
+			inode->i_op = &simple_symlink_inode_operations;
+			inode->i_link = kstrdup(files->link, GFP_KERNEL);
+			if (!inode->i_link) {
+				iput(inode);
+				dput(dentry);
+				goto out;
+			}
+		} else {
+			inode->i_mode = S_IFREG | files->mode;
+			inode->i_fop = files->ops;
+		}
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
-		inode->i_fop = files->ops;
 		inode->i_ino = i;
 		d_add(dentry, inode);
 	}
include/linux/fs.h +0 -0
index dd28814..20c54a2 100644

include/linux/fs.h extern const struct file_operations simple_dir_operations; · line 2959

Quoted 8 lines
include/linux/fs.h +1 -1
@@ -2953,7 +2953,7 @@ extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 extern void make_empty_dir_inode(struct inode *inode);
 extern bool is_empty_dir_inode(struct inode *inode);
-struct tree_descr { char *name; const struct file_operations *ops; int mode; };
+struct tree_descr { char *name; const struct file_operations *ops; int mode; char *link; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
 extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#4 lore raw

PATCH v3 2/2 xenfs: replace xenbus and privcmd with symlinks

In reply to David Vrabel

/proc/xen/xenbus does not work correctly. A read blocked waiting for a xenstore message holds the mutex needed for atomic file position updates. This blocks any writes on the same file handle, which can deadlock if the write is needed to unblock the read.

/proc/xen/xenbus is supposed to be identical to the character device /dev/xen/xenbus so replace the file with a symlink.

Similarly, replace /proc/xen/privcmd with a symlink since it should be the same as /dev/xen/privcmd.

---
v2:
- remove unneeded includes

---
 drivers/xen/xenfs/super.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/xen/xenfs/super.c +4 -6
index 8559a71..0f2e2cd 100644
@@ -18,8 +18,6 @@
 #include <xen/xen.h>
 
 #include "xenfs.h"
-#include "../privcmd.h"
-#include "../xenbus/xenbus_comms.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -45,16 +43,16 @@ static const struct file_operations capabilities_file_ops = {
 static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr xenfs_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{""},
 	};
 
 	static struct tree_descr xenfs_init_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{ "xsd_kva", &xsd_kva_file_ops, S_IRUSR|S_IWUSR},
 		{ "xsd_port", &xsd_port_file_ops, S_IRUSR|S_IWUSR},
 #ifdef CONFIG_XEN_SYMS
Signature · 7 lines
2.1.4


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
maintainer
#5 lore raw

Re: [PATCHv3 2/2] xenfs: replace xenbus and privcmd with symlinks

Quoted 8 lines · David Vrabel

/proc/xen/xenbus does not work correctly. A read blocked waiting for a xenstore message holds the mutex needed for atomic file position updates. This blocks any writes on the same file handle, which can deadlock if the write is needed to unblock the read.

/proc/xen/xenbus is supposed to be identical to the character device /dev/xen/xenbus so replace the file with a symlink.

Quoted 8 lines

Similarly, replace /proc/xen/privcmd with a symlink since it should be the same as /dev/xen/privcmd.

---
v2:
- remove unneeded includes
---

I think you should make xen_xenbus_fops and xen_privcmd_fops static now that they are no longer referenced by super.c

Juergen

Quoted 38 lines
 drivers/xen/xenfs/super.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/xen/xenfs/super.c +4 -6
index 8559a71..0f2e2cd 100644
@@ -18,8 +18,6 @@
 #include <xen/xen.h>
 
 #include "xenfs.h"
-#include "../privcmd.h"
-#include "../xenbus/xenbus_comms.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -45,16 +43,16 @@ static const struct file_operations capabilities_file_ops = {
 static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr xenfs_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{""},
 	};
 
 	static struct tree_descr xenfs_init_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{ "xsd_kva", &xsd_kva_file_ops, S_IRUSR|S_IWUSR},
 		{ "xsd_port", &xsd_port_file_ops, S_IRUSR|S_IWUSR},
 #ifdef CONFIG_XEN_SYMS
maintainer
#6 lore raw

Re: [PATCHv3 2/2] xenfs: replace xenbus and privcmd with symlinks

In reply to David Vrabel
Quoted 8 lines · David Vrabel

/proc/xen/xenbus does not work correctly. A read blocked waiting for a xenstore message holds the mutex needed for atomic file position updates. This blocks any writes on the same file handle, which can deadlock if the write is needed to unblock the read.

/proc/xen/xenbus is supposed to be identical to the character device /dev/xen/xenbus so replace the file with a symlink.

Quoted 8 lines

Similarly, replace /proc/xen/privcmd with a symlink since it should be the same as /dev/xen/privcmd.

---
v2:
- remove unneeded includes
---

I think you should make xen_xenbus_fops and xen_privcmd_fops static now that they are no longer referenced by super.c

Juergen

Quoted 28 lines
 drivers/xen/xenfs/super.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/xen/xenfs/super.c +2 -4
index 8559a71..0f2e2cd 100644
@@ -18,8 +18,6 @@
 #include <xen/xen.h>
 
 #include "xenfs.h"
-#include "../privcmd.h"
-#include "../xenbus/xenbus_comms.h"
 
 #include <asm/xen/hypervisor.h>
 
@@ -45,16 +43,16 @@ static const struct file_operations capabilities_file_ops = {
 static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	static struct tree_descr xenfs_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{""},
 	};

drivers/xen/xenfs/super.c static const struct file_operations capabilities_file_ops = { · line 58

Quoted 10 lines
drivers/xen/xenfs/super.c +2 -2
 
 	static struct tree_descr xenfs_init_files[] = {
-		[2] = { "xenbus", &xen_xenbus_fops, S_IRUSR|S_IWUSR },
+		[2] = { "xenbus", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/xenbus" },
 		{ "capabilities", &capabilities_file_ops, S_IRUGO },
-		{ "privcmd", &xen_privcmd_fops, S_IRUSR|S_IWUSR },
+		{ "privcmd", NULL, S_IFLNK | S_IRWXUGO, "/dev/xen/privcmd" },
 		{ "xsd_kva", &xsd_kva_file_ops, S_IRUSR|S_IWUSR},
 		{ "xsd_port", &xsd_port_file_ops, S_IRUSR|S_IWUSR},
 #ifdef CONFIG_XEN_SYMS

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

#7 lore raw

Re: [Xen-devel] [PATCHv3 0/2] libfs, xenfs: replace /proc/xen/xenbus with a symlink

In reply to David Vrabel
Quoted 5 lines · David Vrabel

Using /proc/xen/xenbus can cause deadlocks on the atomic file position mutex since this file should behave like a character device and not a regular file. This is easiest to achive by making it a symlink to the existing /dev/xen/xenbus device.

Quoted 7 lines

This requires extending simple_fill_super() to add symlinks as well as regular files.

Changes in v3:
- Rebased on v4.7-rc5.

David

Just a bit of a nudge to see where the status of this is at? We're at the point of potentially having yet another kernel release go out the door where the default behavior with Xen 4.6 and earlier is to deadlock domUs.

Signature · 1 line
Doug Goldstein
#8 lore raw

Re: [PATCHv3 0/2] libfs, xenfs: replace /proc/xen/xenbus with a symlink

In reply to David Vrabel
Quoted 5 lines · David Vrabel

Using /proc/xen/xenbus can cause deadlocks on the atomic file position mutex since this file should behave like a character device and not a regular file. This is easiest to achive by making it a symlink to the existing /dev/xen/xenbus device.

Quoted 7 lines

This requires extending simple_fill_super() to add symlinks as well as regular files.

Changes in v3:
- Rebased on v4.7-rc5.

David

Just a bit of a nudge to see where the status of this is at? We're at the point of potentially having yet another kernel release go out the door where the default behavior with Xen 4.6 and earlier is to deadlock domUs.

Signature · 1 line
Doug Goldstein

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel