xfs
[Top] [All Lists]

Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files

To: Zhi Yong Wu <zwu.kernel@xxxxxxxxx>
Subject: Re: [RFC PATCH 4/4] xfs: allow linkat() on O_TMPFILE files
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 28 Nov 2013 02:39:30 -0800
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>, xfstests <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <CAEH94LiiRgQGzf+YGy23SJDhHJ_1grC3kniLqXDdUQFJ09McTA@xxxxxxxxxxxxxx>
References: <1385379154-3802-1-git-send-email-zwu.kernel@xxxxxxxxx> <1385379154-3802-5-git-send-email-zwu.kernel@xxxxxxxxx> <20131125135149.GD30189@xxxxxxxxxxxxx> <CAEH94LiiRgQGzf+YGy23SJDhHJ_1grC3kniLqXDdUQFJ09McTA@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Nov 28, 2013 at 10:37:29AM +0800, Zhi Yong Wu wrote:
> On Mon, Nov 25, 2013 at 9:51 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> >> -     ASSERT(ip->i_d.di_nlink > 0);
> >> +     if ((VFS_I(ip)->i_nlink == 0) &&
> >> +         !(VFS_I(ip)->i_state & I_LINKABLE))
> >> +             ASSERT(ip->i_d.di_nlink > 0);
> >
> >         ASSERT(ip->i_d.di_nlink > 0 || (VFS_I(ip)->i_state & I_LINKABLE));
> This is wrong, and it should be
>           ASSERT(ip->i_d.di_nlink > 0 || !(VFS_I(ip)->i_state & I_LINKABLE));

Why we want to assrrt that either the link count is bigger than 0,
or that the I_LINKABLE flag is set (for files created using O_TMPFILE)

> >> +
> >> +     if ((VFS_I(sip)->i_nlink == 0) &&
> >> +         (VFS_I(sip)->i_state & I_LINKABLE))
> >> +             tres = &M_RES(mp)->tr_link_tmpfile;
> >> +     else
> >> +             tres = &M_RES(mp)->tr_link;
> >
> > Just check i_nlink, and for consistency it might make sense to just use
> > the xfs_inode one.  The VFS already made sure we don't inodes with
> but struct xfs_inode has no stuff similar to i_nlink....

ip->i_d.di_nlink is the equivalent.

> > I_LINKABLE and a zero link count.
> No, pls see the chunk of code:
> int vfs_link(struct dentry *old_dentry, struct inode *dir, struct
> dentry *new_dentry)
> {
> ...
>     /* Make sure we don't allow creating hardlink to an unlinked file */
>     if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
>         error =  -ENOENT;

This makes sure we never created a link if the count is zero unless
the I_LINKABLE is set, so we'll never see a zero link count without
I_LINKABLE.

<Prev in Thread] Current Thread [Next in Thread>