[Ocfs2-devel] a patch for ocfs2_link

[Ocfs2-devel] a patch for ocfs2_link

[Tiger Yang]

This patch remove the redundant "i_nlink >= OCFS2_LINK_MAX" check
and add an unlinked directory check.

Singed-off-by: mfasheh

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 0673862..719a8d2 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -643,11 +643,6 @@ static int ocfs2_link(struct dentry *old
                goto bail;
        }

-       if (inode->i_nlink >= OCFS2_LINK_MAX) {
-               err = -EMLINK;
-               goto bail;
-       }
-
        handle = ocfs2_alloc_handle(osb);
        if (handle == NULL) {
                err = -ENOMEM;
@@ -661,6 +656,11 @@ static int ocfs2_link(struct dentry *old
                goto bail;
        }

+       if (!dir->i_nlink) {
+               err = -ENOENT;
+               goto bail;
+       }
+
        err = ocfs2_check_dir_for_entry(dir, dentry->d_name.name,
                                        dentry->d_name.len);
        if (err)

-------------- next part --------------
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 0673862..719a8d2 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -643,11 +643,6 @@ static int ocfs2_link(struct dentry *old
 		goto bail;
 	}
 
-	if (inode->i_nlink >= OCFS2_LINK_MAX) {
-		err = -EMLINK;
-		goto bail;
-	}
-
 	handle = ocfs2_alloc_handle(osb);
 	if (handle == NULL) {
 		err = -ENOMEM;
@@ -661,6 +656,11 @@ static int ocfs2_link(struct dentry *old
 		goto bail;
 	}
 
+	if (!dir->i_nlink) {
+		err = -ENOENT;
+		goto bail;
+	}
+
 	err = ocfs2_check_dir_for_entry(dir, dentry->d_name.name,
 					dentry->d_name.len);
 	if (err)

[Ocfs2-devel] a patch for ocfs2_link

[Joel Becker]

On Fri, Sep 01, 2006 at 10:27:32AM +0800, Tiger Yang wrote:
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 0673862..719a8d2 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -643,11 +643,6 @@ static int ocfs2_link(struct dentry *old
>                goto bail;
>        }
> 
> -       if (inode->i_nlink >= OCFS2_LINK_MAX) {
> -               err = -EMLINK;
> -               goto bail;
> -       }
> -

	Why is this redundant?  Is someone doing it for us?  I see we
check the fe->i_links_count below, but that's after the expense of a
cluster lock.  Don't we save some effort here?

> @@ -661,6 +656,11 @@ static int ocfs2_link(struct dentry *old
>                goto bail;
>        }
> 
> +       if (!dir->i_nlink) {
> +               err = -ENOENT;
> +               goto bail;
> +       }
> +

	And what does this do?  If a directory...oh, wait, is this to
check for a directory that's going away?  Can this actually happen?  I'd
think that the VFS would protect it.

Joel

-- 

"Well-timed silence hath more eloquence than speech."  
         - Martin Fraquhar Tupper

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] a patch for ocfs2_link

[Mark Fasheh]

On Thu, Aug 31, 2006 at 08:06:33PM -0700, Joel Becker wrote:
> > -       if (inode->i_nlink >= OCFS2_LINK_MAX) {
> > -               err = -EMLINK;
> > -               goto bail;
> > -       }
> > -
> 
> 	Why is this redundant?  Is someone doing it for us?  I see we
> check the fe->i_links_count below, but that's after the expense of a
> cluster lock.  Don't we save some effort here?
It's wrong because the i_nlink might be stale. We might actually be able to
insert, but we don't know until we have the cluster lock.


> > @@ -661,6 +656,11 @@ static int ocfs2_link(struct dentry *old
> >                goto bail;
> >        }
> > 
> > +       if (!dir->i_nlink) {
> > +               err = -ENOENT;
> > +               goto bail;
> > +       }
> > +
> 
> 	And what does this do?  If a directory...oh, wait, is this to
> check for a directory that's going away?  Can this actually happen?  I'd
> think that the VFS would protect it.
Again, the link count might be stale, so we have to double check after
getting the lock. The way I imagine this could happen is if the dir inode is
orphaned and a process attempts to do a create (imagine if it was unlinked
while someone still had a shell open there)

There are similar checks in ocfs2_symlink() and ocfs2_mknod() which Tiger is
basing that one on.


Granted, neither of these conditions are particularly likely, but we should
protect against them nonetheless.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh@oracle.com

[Ocfs2-devel] Re: a patch for ocfs2_link

[Mark Fasheh]

Hi Tiger,

On Fri, Sep 01, 2006 at 10:27:32AM +0800, Tiger Yang wrote:
> This patch remove the redundant "i_nlink >= OCFS2_LINK_MAX" check
> and add an unlinked directory check.
Ok, this looks good - thanks.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh@oracle.com