| Message ID | d91e3660266d3a2956c4d1aebc9cb618081b21ef.1285278339.git.matthltc@us.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 23, 2010 at 02:53:28PM -0700, Matt Helsley wrote: > Not all filesystems will necessarily be able to support relinking an > orphan inode back into the filesystem. Some offlist feedback suggested > that instead of overloading .link that relinking should be a separate > file operation for this reason. > > Since .relink is a superset of .link make the VFS call .relink where > possible and .link otherwise. > > The next commit will change ext3/4 to enable this operation. I may have missed something in one of these patches (patch 1 and any original summary if there was one don't appear in my email), but what is the point of the new operation? I didn't see any case that treats one any different than the other. What is disallowed (and how) for a driver which does not implement .relink but has .link? Brad Boyer flar@allandria.com -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 26, 2010 at 12:08:37PM -0700, Brad Boyer wrote: > On Thu, Sep 23, 2010 at 02:53:28PM -0700, Matt Helsley wrote: > > Not all filesystems will necessarily be able to support relinking an > > orphan inode back into the filesystem. Some offlist feedback suggested > > that instead of overloading .link that relinking should be a separate > > file operation for this reason. > > > > Since .relink is a superset of .link make the VFS call .relink where > > possible and .link otherwise. > > > > The next commit will change ext3/4 to enable this operation. > > I may have missed something in one of these patches (patch 1 and any > original summary if there was one don't appear in my email), but > what is the point of the new operation? I didn't see any case that > treats one any different than the other. What is disallowed (and how) > for a driver which does not implement .relink but has .link? Did you get patch 3? It shows how ext3/ext4 add the ability to take an inode that has been unlinked, placed onto the orphan list, and relink it. Cheers, -Matt Helsley -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 27, 2010 at 12:16:28PM -0700, Matt Helsley wrote: > On Sun, Sep 26, 2010 at 12:08:37PM -0700, Brad Boyer wrote: > > On Thu, Sep 23, 2010 at 02:53:28PM -0700, Matt Helsley wrote: > > > Not all filesystems will necessarily be able to support relinking an > > > orphan inode back into the filesystem. Some offlist feedback suggested > > > that instead of overloading .link that relinking should be a separate > > > file operation for this reason. > > > > > > Since .relink is a superset of .link make the VFS call .relink where > > > possible and .link otherwise. > > > > > > The next commit will change ext3/4 to enable this operation. > > > > I may have missed something in one of these patches (patch 1 and any > > original summary if there was one don't appear in my email), but > > what is the point of the new operation? I didn't see any case that > > treats one any different than the other. What is disallowed (and how) > > for a driver which does not implement .relink but has .link? > > Did you get patch 3? It shows how ext3/ext4 add the ability to take an > inode that has been unlinked, placed onto the orphan list, and relink it. Yes, I did get patch 3. I think you misunderstood my question. You point both .link and .relink to the same function in ext3 and ext4. The common code which calls them will call .relink if it is set and .link if it is not set. If nothing acts any different based on .relink being NULL or not-NULL, and the only implementation isn't any different from .link what was the point of introducing a new operation? What I expected to see was that some particular code path would check if .relink was NULL and fail in that case. Unless there is a code path that will only call .relink and not .link, it seems useless to me. Brad Boyer flar@allandria.com -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/27/2010 06:03 PM, Brad Boyer wrote: > On Mon, Sep 27, 2010 at 12:16:28PM -0700, Matt Helsley wrote: >> On Sun, Sep 26, 2010 at 12:08:37PM -0700, Brad Boyer wrote: >>> On Thu, Sep 23, 2010 at 02:53:28PM -0700, Matt Helsley wrote: >>>> Not all filesystems will necessarily be able to support relinking an >>>> orphan inode back into the filesystem. Some offlist feedback suggested >>>> that instead of overloading .link that relinking should be a separate >>>> file operation for this reason. In light of Brad's comment (below), maybe elaborate on this: Some offlist feedback suggested that instead of overloading .link to provide this functionality, relinking of an orphan inode back into the filesystem should be a separate file operation. This is because some filesystems may not be able to support this operation. Their existing .link already assumes that the inode isn't orphan (i_nlink != 0), but still won't explicitly test for the condition. If this is the case, the overloading .link may break their assumptions and a call to relink may not be handled too gracefully. >>>> >>>> Since .relink is a superset of .link make the VFS call .relink where >>>> possible and .link otherwise. >>>> >>>> The next commit will change ext3/4 to enable this operation. >>> >>> I may have missed something in one of these patches (patch 1 and any >>> original summary if there was one don't appear in my email), but >>> what is the point of the new operation? I didn't see any case that >>> treats one any different than the other. What is disallowed (and how) >>> for a driver which does not implement .relink but has .link? >> >> Did you get patch 3? It shows how ext3/ext4 add the ability to take an >> inode that has been unlinked, placed onto the orphan list, and relink it. > > Yes, I did get patch 3. I think you misunderstood my question. You point > both .link and .relink to the same function in ext3 and ext4. The common > code which calls them will call .relink if it is set and .link if it is > not set. If nothing acts any different based on .relink being NULL or > not-NULL, and the only implementation isn't any different from .link > what was the point of introducing a new operation? > > What I expected to see was that some particular code path would check > if .relink was NULL and fail in that case. Unless there is a code path > that will only call .relink and not .link, it seems useless to me. Does the above help clarify this ? The test performed in vfs_link(). Oren. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/23/2010 05:53 PM, Matt Helsley wrote: > Not all filesystems will necessarily be able to support relinking an > orphan inode back into the filesystem. Some offlist feedback suggested > that instead of overloading .link that relinking should be a separate > file operation for this reason. > > Since .relink is a superset of .link make the VFS call .relink where > possible and .link otherwise. > > The next commit will change ext3/4 to enable this operation. > > Signed-off-by: Matt Helsley<matthltc@us.ibm.com> > Cc: Theodore Ts'o<tytso@mit.edu> > Cc: Andreas Dilger<adilger.kernel@dilger.ca> > Cc: Jan Kara<jack@suse.cz> > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-ext4@vger.kernel.org > Cc: Al Viro<viro@zeniv.linux.org.uk> > --- > fs/namei.c | 5 ++++- > include/linux/fs.h | 1 + > 2 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index a7dce91..eb279e3 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2446,7 +2446,10 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de > return error; > > mutex_lock(&inode->i_mutex); > - error = dir->i_op->link(old_dentry, dir, new_dentry); > + if (dir->i_op->relink) > + error = dir->i_op->relink(old_dentry, dir, new_dentry); > + else > + error = dir->i_op->link(old_dentry, dir, new_dentry); Can there be a scenario/filesystem in which .relink implementation is so much more complex (and expensive) than .link ? If the answer is "yes", then this we probably don't want to do this, and let vfs_link() call .link, and instead add a new helper vfs_relink(). Oren. > mutex_unlock(&inode->i_mutex); > if (!error) > fsnotify_link(dir, inode, new_dentry); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index ee725ff..d932eb7 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1528,6 +1528,7 @@ struct inode_operations { > int (*create) (struct inode *,struct dentry *,int, struct nameidata *); > struct dentry * (*lookup) (struct inode *,struct dentry *, struct nameidata *); > int (*link) (struct dentry *,struct inode *,struct dentry *); > + int (*relink) (struct dentry *,struct inode *,struct dentry *); > int (*unlink) (struct inode *,struct dentry *); > int (*symlink) (struct inode *,struct dentry *,const char *); > int (*mkdir) (struct inode *,struct dentry *,int); -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 29, 2010 at 04:19:31PM -0400, Oren Laadan wrote: > > > On 09/23/2010 05:53 PM, Matt Helsley wrote: > >Not all filesystems will necessarily be able to support relinking an > >orphan inode back into the filesystem. Some offlist feedback suggested > >that instead of overloading .link that relinking should be a separate > >file operation for this reason. > > > >Since .relink is a superset of .link make the VFS call .relink where > >possible and .link otherwise. > > > >The next commit will change ext3/4 to enable this operation. > > > >Signed-off-by: Matt Helsley<matthltc@us.ibm.com> > >Cc: Theodore Ts'o<tytso@mit.edu> > >Cc: Andreas Dilger<adilger.kernel@dilger.ca> > >Cc: Jan Kara<jack@suse.cz> > >Cc: linux-fsdevel@vger.kernel.org > >Cc: linux-ext4@vger.kernel.org > >Cc: Al Viro<viro@zeniv.linux.org.uk> > >--- > > fs/namei.c | 5 ++++- > > include/linux/fs.h | 1 + > > 2 files changed, 5 insertions(+), 1 deletions(-) > > > >diff --git a/fs/namei.c b/fs/namei.c > >index a7dce91..eb279e3 100644 > >--- a/fs/namei.c > >+++ b/fs/namei.c > >@@ -2446,7 +2446,10 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de > > return error; > > > > mutex_lock(&inode->i_mutex); > >- error = dir->i_op->link(old_dentry, dir, new_dentry); > >+ if (dir->i_op->relink) > >+ error = dir->i_op->relink(old_dentry, dir, new_dentry); > >+ else > >+ error = dir->i_op->link(old_dentry, dir, new_dentry); > > Can there be a scenario/filesystem in which .relink implementation > is so much more complex (and expensive) than .link ? > > If the answer is "yes", then this we probably don't want to do > this, and let vfs_link() call .link, and instead add a new helper > vfs_relink(). OK, that makes some sense too. I thought the separation would just be at the file operations layer but we can move it higher too. I'll adjust the patches to do that and repost them. Cheers, -Matt -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/namei.c b/fs/namei.c index a7dce91..eb279e3 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2446,7 +2446,10 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de return error; mutex_lock(&inode->i_mutex); - error = dir->i_op->link(old_dentry, dir, new_dentry); + if (dir->i_op->relink) + error = dir->i_op->relink(old_dentry, dir, new_dentry); + else + error = dir->i_op->link(old_dentry, dir, new_dentry); mutex_unlock(&inode->i_mutex); if (!error) fsnotify_link(dir, inode, new_dentry); diff --git a/include/linux/fs.h b/include/linux/fs.h index ee725ff..d932eb7 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1528,6 +1528,7 @@ struct inode_operations { int (*create) (struct inode *,struct dentry *,int, struct nameidata *); struct dentry * (*lookup) (struct inode *,struct dentry *, struct nameidata *); int (*link) (struct dentry *,struct inode *,struct dentry *); + int (*relink) (struct dentry *,struct inode *,struct dentry *); int (*unlink) (struct inode *,struct dentry *); int (*symlink) (struct inode *,struct dentry *,const char *); int (*mkdir) (struct inode *,struct dentry *,int);
Not all filesystems will necessarily be able to support relinking an orphan inode back into the filesystem. Some offlist feedback suggested that instead of overloading .link that relinking should be a separate file operation for this reason. Since .relink is a superset of .link make the VFS call .relink where possible and .link otherwise. The next commit will change ext3/4 to enable this operation. Signed-off-by: Matt Helsley <matthltc@us.ibm.com> Cc: Theodore Ts'o <tytso@mit.edu> Cc: Andreas Dilger <adilger.kernel@dilger.ca> Cc: Jan Kara <jack@suse.cz> Cc: linux-fsdevel@vger.kernel.org Cc: linux-ext4@vger.kernel.org Cc: Al Viro <viro@zeniv.linux.org.uk> --- fs/namei.c | 5 ++++- include/linux/fs.h | 1 + 2 files changed, 5 insertions(+), 1 deletions(-)