Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 6 May 2020 08:14:07 +0800
From:   Gao Xiang <hsiangkao@gmx.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Jaegeuk Kim <jaegeuk@kernel.org>, kernel-team@android.com,
        linux-kernel@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino
Message-ID: <20200506001403.GA2101@hsiangkao-HP-ZHAN-66-Pro-G1>
References: <20200505153139.201697-1-jaegeuk@kernel.org>
 <20200505165847.GA98848@gmail.com>
 <20200505181323.GA55221@google.com>
 <20200505181941.GC98848@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200505181941.GC98848@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Eric,

On Tue, May 05, 2020 at 11:19:41AM -0700, Eric Biggers wrote:
> On Tue, May 05, 2020 at 11:13:23AM -0700, Jaegeuk Kim wrote:

...

> >
> > -static int get_parent_ino(struct inode *inode, nid_t *pino)
> > -{
> > -	struct dentry *dentry;
> > -
> > -	inode =3D igrab(inode);
> > -	dentry =3D d_find_any_alias(inode);
> > -	iput(inode);
> > -	if (!dentry)
> > -		return 0;
> > -
> > -	*pino =3D parent_ino(dentry);
> > -	dput(dentry);
> > -	return 1;
> > -}
> > -
> >  static inline enum cp_reason_type need_do_checkpoint(struct inode *in=
ode)
> >  {
> >  	struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);
> > @@ -223,14 +208,15 @@ static bool need_inode_page_update(struct f2fs_s=
b_info *sbi, nid_t ino)
> >  	return ret;
> >  }
> >
> > -static void try_to_fix_pino(struct inode *inode)
> > +static void try_to_fix_pino(struct dentry *dentry)
> >  {
> > +	struct inode *inode =3D d_inode(dentry);
> >  	struct f2fs_inode_info *fi =3D F2FS_I(inode);
> > -	nid_t pino;
> >
> >  	down_write(&fi->i_sem);
> > -	if (file_wrong_pino(inode) && inode->i_nlink =3D=3D 1 &&
> > -			get_parent_ino(inode, &pino)) {
> > +	if (file_wrong_pino(inode) && inode->i_nlink =3D=3D 1) {
> > +		nid_t pino =3D parent_ino(dentry);
> > +
> >  		f2fs_i_pino_write(inode, pino);
> >  		file_got_pino(inode);
> >  	}
> > @@ -310,7 +296,7 @@ static int f2fs_do_sync_file(struct file *file, lo=
ff_t start, loff_t end,
> >  		 * We've secured consistency through sync_fs. Following pino
> >  		 * will be used only for fsynced inodes after checkpoint.
> >  		 */
> > -		try_to_fix_pino(inode);
> > +		try_to_fix_pino(file_dentry(file));
> >  		clear_inode_flag(inode, FI_APPEND_WRITE);
> >  		clear_inode_flag(inode, FI_UPDATE_WRITE);
> >  		goto out;
>
> Actually, I think this is wrong because the fsync can be done via a file
> descriptor that was opened to a now-deleted link to the file.

I'm still confused about this...

I don't know what's wrong with this version from my limited knowledge?
 inode itself is locked when fsyncing, so

   if the fsync inode->i_nlink =3D=3D 1, this inode has only one hard link
   (not deleted yet) and should belong to a single directory; and

   the only one parent directory would not go away (not deleted as well)
   since there are some dirents in it (not empty).

Could kindly explain more so I would learn more about this scenario?
Thanks a lot!

>
> We need to find the dentry whose parent directory is still exists, i.e. =
the
> parent directory that is counting towards 'inode->i_nlink =3D=3D 1'.

directory counting towards 'inode->i_nlink =3D=3D 1', what's happening?

>
> I think d_find_alias() is what we're looking for.

It may be simply dentry->d_parent (stable/positive as you said before, and=
 it's
not empty). why need to d_find_alias()?


And what is the original problem? I could not get some clue from the origi=
nal
patch description (I only saw some extra igrab/iput because of some unknow=
n
reasons), it there some backtrace related to the problem?

Thanks,
Gao Xiang

>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 6ab8f621a3c5..855f27468baa 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -165,13 +165,13 @@ static int get_parent_ino(struct inode *inode, nid=
_t *pino)
>  {
>         struct dentry *dentry;
>
> -       inode =3D igrab(inode);
> -       dentry =3D d_find_any_alias(inode);
> -       iput(inode);
> +       dentry =3D d_find_alias(inode);
>         if (!dentry)
>                 return 0;
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel