Messages in this thread | | | Subject | Re: [PATCH/RFC] s390: Hypervisor File System | From | Michael Holzheu <> | Date | Wed, 26 Apr 2006 11:30:11 +0200 |
| |
Hi Pekka,
Pekka Enberg <penberg@cs.helsinki.fi> wrote on 04/25/2006 04:33:01 PM: > On Mon, 2006-04-24 at 19:19 +0200, Michael Holzheu wrote: > > +static int hypfs_create_cpu_files(struct super_block *sb, > > + struct dentry *cpus_dir, void *cpu_info) > > +{ > > + struct dentry *cpu_dir; > > + char buffer[TMP_SIZE]; > > Holy cow! That's 1 KB allocated on the stack! Please use kmalloc() > instead.
Thanks! We also have found that already! Shame on me ...
> > +static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user *buf, > > + size_t count, loff_t pos) > > +{ > > + int rc; > > + > > + mutex_lock(&hypfs_lock); > > + if (last_update_time == get_seconds()) { > > + rc = -EBUSY; > > + goto out; > > + } > > + hypfs_delete_tree(hypfs_sblk->s_root); > > To state what I said earlier: the use of a global hypfs_sblk is > problematic because now we can only have the filesystem mounted once. So > I would really like to see some other way of updating. How do you feel > about the s_ops->fs_remount thing? >
I will eliminate the global variable anyway, since I can get the superblock also from the inode:
+static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user *buf, + size_t count, loff_t pos) +{ + int rc; + struct super_block *sb; + + mutex_lock(&hypfs_lock); + sb = iocb->ki_filp->f_dentry->d_inode->i_sb; + if (last_update_time == get_seconds()) { + rc = -EBUSY; + goto out; + } + hypfs_delete_tree(sb->s_root); + rc = diag_create_files(sb, sb->s_root); + if (rc) { + printk(KERN_ERR "hypfs: Update failed\n"); + hypfs_delete_tree(sb->s_root); + goto out; + } + hypfs_update_update(); + rc = count; + out: + mutex_unlock(&hypfs_lock); + return rc; +}
Hmmm... sure, you could use remount to trigger the update. One advantage of remount is that we do not need the hypfs_lock, right. But having the hypfs lock needs only three lines of code and therefore this is not really a strong argument in my eyes.
I think one disadvantage of the remount mechanism is that it is not as intuitive as the update attribute. If you have an update file, it is clear that writing to that file triggers an update. That a remount triggers an update seems to be less intuitive for me.
But the bigger disadvantage is that only root (or any user if you specify 'user' in /etc/fstab) can trigger the update with remount. We want to allow also other users to trigger the update and read the attributes. The current implementation for that is to specify uid and gid as mount option to define the owner of the files. For example you can run a user space program with uid xxx and specify uid=xxx as mount option to allow the program to update and read the data.
Michael
- 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/
| |