Mailing List Archive

On Thu, 2007-09-27 at 21:26 +0100, Christoph Hellwig wrote:
>
> Dave will probably find a bandaid to work around this, but the
> right fix is to stop using a file struct here entirely. If you
> look at reiserfs_xattr_set it's not actually used at all except
> for passing it to ->prepare_write and ->commit_write which then
> don't use it at all. All that crap should be rewritten to just
> pass the dentry around. Note that all this should not acquire
> writer counts on the vfsmount - we have done this already
> before calling into the xattr methods.

I'm perplexed as to why a 'struct file' is needed, too. The 'struct
file' doesn't get used for anything _but_ the dentry, and the functions
to which it is passed (reiserfs_prepate/commit_write()) don't use it.
In fact, some other callers just pass NULL.

This is a completely naive implementation, and I've only tested that it
compiles, but does anybody see a reason we can't just do this?

BTW, do reiserfs developers know that you can put function declarations
in header files? ;) 4 different .c files have this:

int reiserfs_commit_write(struct page *page, unsigned from, unsigned to);
int reiserfs_prepare_write(struct page *page, unsigned from, unsigned to);

---

lxc-dave/fs/reiserfs/inode.c | 15 ++++------
lxc-dave/fs/reiserfs/ioctl.c | 10 ++-----
lxc-dave/fs/reiserfs/xattr.c | 59 +++++++++++++------------------------------
3 files changed, 28 insertions(+), 56 deletions(-)

diff -puN fs/open.c~fix-reiserfs-oops fs/open.c
diff -puN fs/file_table.c~fix-reiserfs-oops fs/file_table.c
diff -puN include/linux/mount.h~fix-reiserfs-oops include/linux/mount.h
diff -puN fs/reiserfs/xattr.c~fix-reiserfs-oops fs/reiserfs/xattr.c
--- lxc/fs/reiserfs/xattr.c~fix-reiserfs-oops 2007-09-27 13:36:38.000000000 -0700
+++ lxc-dave/fs/reiserfs/xattr.c 2007-09-27 13:49:02.000000000 -0700
@@ -194,27 +194,6 @@ static struct dentry *get_xa_file_dentry
return xafile;
}

-/* Opens a file pointer to the attribute associated with inode */
-static struct file *open_xa_file(const struct inode *inode, const char *name,
- int flags)
-{
- struct dentry *xafile;
- struct file *fp;
-
- xafile = get_xa_file_dentry(inode, name, flags);
- if (IS_ERR(xafile))
- return ERR_PTR(PTR_ERR(xafile));
- else if (!xafile->d_inode) {
- dput(xafile);
- return ERR_PTR(-ENODATA);
- }
-
- fp = dentry_open(xafile, NULL, O_RDWR);
- /* dentry_open dputs the dentry if it fails */
-
- return fp;
-}
-
/*
* this is very similar to fs/reiserfs/dir.c:reiserfs_readdir, but
* we need to drop the path before calling the filldir struct. That
@@ -426,10 +405,8 @@ static inline __u32 xattr_hash(const cha
return csum_partial(msg, len, 0);
}

-int reiserfs_commit_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
-int reiserfs_prepare_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
+int reiserfs_commit_write(struct page *page, unsigned from, unsigned to);
+int reiserfs_prepare_write(struct page *page, unsigned from, unsigned to);


/* Generic extended attribute operations that can be used by xa plugins */
@@ -442,7 +419,7 @@ reiserfs_xattr_set(struct inode *inode,
size_t buffer_size, int flags)
{
int err = 0;
- struct file *fp;
+ struct dentry *dentry;
struct page *page;
char *data;
struct address_space *mapping;
@@ -460,18 +437,18 @@ reiserfs_xattr_set(struct inode *inode,
xahash = xattr_hash(buffer, buffer_size);

open_file:
- fp = open_xa_file(inode, name, flags);
- if (IS_ERR(fp)) {
- err = PTR_ERR(fp);
+ dentry = get_xa_file_dentry(inode, name, flags);
+ if (IS_ERR(dentry)) {
+ err = PTR_ERR(dentry);
goto out;
}

- xinode = fp->f_path.dentry->d_inode;
+ xinode = dentry->d_inode;
REISERFS_I(inode)->i_flags |= i_has_xattr_dir;

/* we need to copy it off.. */
if (xinode->i_nlink > 1) {
- fput(fp);
+ dput(dentry);
err = reiserfs_xattr_del(inode, name);
if (err < 0)
goto out;
@@ -485,7 +462,7 @@ reiserfs_xattr_set(struct inode *inode,
newattrs.ia_size = buffer_size;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
mutex_lock(&xinode->i_mutex);
- err = notify_change(fp->f_path.dentry, &newattrs);
+ err = notify_change(dentry, &newattrs);
if (err)
goto out_filp;

@@ -518,13 +495,13 @@ reiserfs_xattr_set(struct inode *inode,
rxh->h_hash = cpu_to_le32(xahash);
}

- err = reiserfs_prepare_write(fp, page, page_offset,
+ err = reiserfs_prepare_write(page, page_offset,
page_offset + chunk + skip);
if (!err) {
if (buffer)
memcpy(data + skip, buffer + buffer_pos, chunk);
err =
- reiserfs_commit_write(fp, page, page_offset,
+ reiserfs_commit_write(page, page_offset,
page_offset + chunk +
skip);
}
@@ -548,7 +525,7 @@ reiserfs_xattr_set(struct inode *inode,

out_filp:
mutex_unlock(&xinode->i_mutex);
- fput(fp);
+ dput(dentry);

out:
return err;
@@ -562,7 +539,7 @@ reiserfs_xattr_get(const struct inode *i
size_t buffer_size)
{
ssize_t err = 0;
- struct file *fp;
+ struct dentry *dentry;
size_t isize;
size_t file_pos = 0;
size_t buffer_pos = 0;
@@ -578,13 +555,13 @@ reiserfs_xattr_get(const struct inode *i
if (get_inode_sd_version(inode) == STAT_DATA_V1)
return -EOPNOTSUPP;

- fp = open_xa_file(inode, name, FL_READONLY);
- if (IS_ERR(fp)) {
- err = PTR_ERR(fp);
+ dentry = get_xa_file_dentry(inode, name, FL_READONLY);
+ if (IS_ERR(dentry)) {
+ err = PTR_ERR(dentry);
goto out;
}

- xinode = fp->f_path.dentry->d_inode;
+ xinode = dentry->d_inode;
isize = xinode->i_size;
REISERFS_I(inode)->i_flags |= i_has_xattr_dir;

@@ -652,7 +629,7 @@ reiserfs_xattr_get(const struct inode *i
}

out_dput:
- fput(fp);
+ dput(dentry);

out:
return err;
diff -puN fs/reiserfs/inode.c~fix-reiserfs-oops fs/reiserfs/inode.c
--- lxc/fs/reiserfs/inode.c~fix-reiserfs-oops 2007-09-27 13:39:47.000000000 -0700
+++ lxc-dave/fs/reiserfs/inode.c 2007-09-27 13:45:03.000000000 -0700
@@ -19,10 +19,8 @@
#include <linux/quotaops.h>
#include <linux/swap.h>

-int reiserfs_commit_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
-int reiserfs_prepare_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
+int reiserfs_commit_write(struct page *page, unsigned from, unsigned to);
+int reiserfs_prepare_write(struct page *page, unsigned from, unsigned to);

void reiserfs_delete_inode(struct inode *inode)
{
@@ -550,14 +548,14 @@ static int convert_tail_for_hole(struct
** won't trigger a get_block in this case.
*/
fix_tail_page_for_writing(tail_page);
- retval = reiserfs_prepare_write(NULL, tail_page, tail_start, tail_end);
+ retval = reiserfs_prepare_write(tail_page, tail_start, tail_end);
if (retval)
goto unlock;

/* tail conversion might change the data in the page */
flush_dcache_page(tail_page);

- retval = reiserfs_commit_write(NULL, tail_page, tail_start, tail_end);
+ retval = reiserfs_commit_write(tail_page, tail_start, tail_end);

unlock:
if (tail_page != hole_page) {
@@ -2613,8 +2611,7 @@ static int reiserfs_write_begin(struct f
return ret;
}

-int reiserfs_prepare_write(struct file *f, struct page *page,
- unsigned from, unsigned to)
+int reiserfs_prepare_write(struct page *page, unsigned from, unsigned to)
{
struct inode *inode = page->mapping->host;
int ret;
@@ -2761,7 +2758,7 @@ static int reiserfs_write_end(struct fil
goto out;
}

-int reiserfs_commit_write(struct file *f, struct page *page,
+int reiserfs_commit_write(struct page *page,
unsigned from, unsigned to)
{
struct inode *inode = page->mapping->host;
diff -puN fs/reiserfs/ioctl.c~fix-reiserfs-oops fs/reiserfs/ioctl.c
--- lxc/fs/reiserfs/ioctl.c~fix-reiserfs-oops 2007-09-27 13:42:07.000000000 -0700
+++ lxc-dave/fs/reiserfs/ioctl.c 2007-09-27 13:45:17.000000000 -0700
@@ -143,10 +143,8 @@ long reiserfs_compat_ioctl(struct file *
}
#endif

-int reiserfs_commit_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
-int reiserfs_prepare_write(struct file *f, struct page *page,
- unsigned from, unsigned to);
+int reiserfs_commit_write(struct page *page, unsigned from, unsigned to);
+int reiserfs_prepare_write(struct page *page, unsigned from, unsigned to);
/*
** reiserfs_unpack
** Function try to convert tail from direct item into indirect.
@@ -194,13 +192,13 @@ static int reiserfs_unpack(struct inode
if (!page) {
goto out;
}
- retval = reiserfs_prepare_write(NULL, page, write_from, write_from);
+ retval = reiserfs_prepare_write(page, write_from, write_from);
if (retval)
goto out_unlock;

/* conversion can change page contents, must flush */
flush_dcache_page(page);
- retval = reiserfs_commit_write(NULL, page, write_from, write_from);
+ retval = reiserfs_commit_write(page, write_from, write_from);
REISERFS_I(inode)->i_flags |= i_nopack_mask;

out_unlock:
_


-- Dave

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