Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-28 Thread Christoph Hellwig 
On Thu, Sep 27, 2007 at 02:51:25PM -0700, Andrew Morton wrote:
 That'll be because reiserfs-convert-to-new-aops.patch witched reiserfs over
 to -write_begin() and -write_end().

Yeah, I was looking at mainline.

 So your stuff becomes dependent on Nick's stuff, and Nick's stuff is still
 failing on NFS, I think.

I'd rather avoid the paramater removal for now, that makes it less
entangle, and it's an unrelated cleanup anyway.

Btw, there's more abuse of this sort in reiserfs.  Various other places
in xattr.c call dentry_open directly without the vfsmount aswell.  And
handling of an external journal uses filp_open which is similarly stupid,
it should use open_bdev_excl like xfs or the generic code to open the
main filesystem blockdevice.

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

[RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Dave Hansen 
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.0 
-0700
+++ lxc-dave/fs/reiserfs/xattr.c2007-09-27 13:49:02.0 -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,

Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Christoph Hellwig 
On Thu, Sep 27, 2007 at 01:53:39PM -0700, Dave Hansen wrote:
 -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);

I doubt this will work.  These are also used for the -prepare_write
and -commit_write aops, and the method signature definitively wants
a file there, even if it's zero..

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

Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Dave Hansen 
On Thu, 2007-09-27 at 22:04 +0100, Christoph Hellwig wrote:
 On Thu, Sep 27, 2007 at 01:53:39PM -0700, Dave Hansen wrote:
  -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);
 
 I doubt this will work.  These are also used for the -prepare_write
 and -commit_write aops, and the method signature definitively wants
 a file there, even if it's zero..

Oddly enough, I don't see those functions being used in aops:

const struct address_space_operations reiserfs_address_space_operations = {
.writepage = reiserfs_writepage,
.readpage = reiserfs_readpage,
.readpages = reiserfs_readpages,
.releasepage = reiserfs_releasepage,
.invalidatepage = reiserfs_invalidatepage,
.sync_page = block_sync_page,
.write_begin = reiserfs_write_begin,
.write_end = reiserfs_write_end,
.bmap = reiserfs_aop_bmap,
.direct_IO = reiserfs_direct_IO,
.set_page_dirty = reiserfs_set_page_dirty,
};

Plus, reiserfs seems to compile with that patch I just sent.  Sure as
heck surprised me.

-- Dave

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

Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Andrew Morton 
On Thu, 27 Sep 2007 14:27:14 -0700
Dave Hansen [EMAIL PROTECTED] wrote:

 On Thu, 2007-09-27 at 22:04 +0100, Christoph Hellwig wrote:
  On Thu, Sep 27, 2007 at 01:53:39PM -0700, Dave Hansen wrote:
   -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);
  
  I doubt this will work.  These are also used for the -prepare_write
  and -commit_write aops, and the method signature definitively wants
  a file there, even if it's zero..
 
 Oddly enough, I don't see those functions being used in aops:
 
 const struct address_space_operations reiserfs_address_space_operations = {
 .writepage = reiserfs_writepage,
 .readpage = reiserfs_readpage,
 .readpages = reiserfs_readpages,
 .releasepage = reiserfs_releasepage,
 .invalidatepage = reiserfs_invalidatepage,
 .sync_page = block_sync_page,
 .write_begin = reiserfs_write_begin,
 .write_end = reiserfs_write_end,
 .bmap = reiserfs_aop_bmap,
 .direct_IO = reiserfs_direct_IO,
 .set_page_dirty = reiserfs_set_page_dirty,
 };
 
 Plus, reiserfs seems to compile with that patch I just sent.  Sure as
 heck surprised me.
 

That'll be because reiserfs-convert-to-new-aops.patch witched reiserfs over
to -write_begin() and -write_end().

So your stuff becomes dependent on Nick's stuff, and Nick's stuff is still
failing on NFS, I think.


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

Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Andrew Morton 
On Thu, 27 Sep 2007 14:51:25 -0700
Andrew Morton [EMAIL PROTECTED] wrote:

  Plus, reiserfs seems to compile with that patch I just sent.  Sure as
  heck surprised me.
  
 
 That'll be because reiserfs-convert-to-new-aops.patch witched reiserfs over
 to -write_begin() and -write_end().

Actually, we should rename reiserfs_prepare_write and reiserfs_commit_write
to something else to reduce confusion.  Probably lots of other filesystems
would benefit from the same change, post-Nick's-stuff.
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] make reiserfs stop using 'struct file' for internal xattr operations

2007-09-27 Thread Peter Zijlstra 

On Thu, 2007-09-27 at 14:51 -0700, Andrew Morton wrote:

 So your stuff becomes dependent on Nick's stuff, and Nick's stuff is still
 failing on NFS, I think.

It worked today, it turned out to be a UML bug. Real hardware seemed to
work properly, but will test a bit more tomorrow.

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