[PATCH] f2fs: update file name in the inode block during f2fs_rename

[PATCH] f2fs: update file name in the inode block during f2fs_rename

[Jaegeuk Kim]

The error is reproducible by:

After this, when we retrieve the inode->i_name of test2 by dump.f2fs, we get
test1 instead of test2.
This is because f2fs didn't update the file name during the f2fs_rename.

So, this patch fixes that.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/dir.c   | 15 +++++++++++++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/namei.c |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 89ecb37..d1bb260 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -276,6 +276,21 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
 	set_page_dirty(ipage);
 }
 
+int update_dent_inode(struct inode *inode, const struct qstr *name)
+{
+	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
+	struct page *page;
+
+	page = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(page))
+		return PTR_ERR(page);
+
+	init_dent_inode(name, page);
+	f2fs_put_page(page, 1);
+
+	return 0;
+}
+
 static int make_empty_dir(struct inode *inode,
 		struct inode *parent, struct page *page)
 {
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 2dfd584..a6858c7 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -953,6 +953,7 @@ struct f2fs_dir_entry *f2fs_parent_dir(struct inode *, struct page **);
 ino_t f2fs_inode_by_name(struct inode *, struct qstr *);
 void f2fs_set_link(struct inode *, struct f2fs_dir_entry *,
 				struct page *, struct inode *);
+int update_dent_inode(struct inode *, const struct qstr *);
 int __f2fs_add_link(struct inode *, const struct qstr *, struct inode *);
 void f2fs_delete_entry(struct f2fs_dir_entry *, struct page *, struct inode *);
 int f2fs_make_empty(struct inode *, struct inode *);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 64c0716..3297278 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -427,6 +427,11 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		if (!new_entry)
 			goto out_dir;
 
+		if (update_dent_inode(old_inode, &new_dentry->d_name)) {
+			f2fs_put_page(new_page, 1);
+			goto out_dir;
+		}
+
 		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
 
 		new_inode->i_ctime = CURRENT_TIME;
-- 
1.8.3.1.437.g0dbd812

Re: [PATCH] f2fs: update file name in the inode block during f2fs_rename

[Al Viro]

On Thu, Jul 18, 2013 at 06:11:23PM +0900, Jaegeuk Kim wrote:
> The error is reproducible by:
> 
> After this, when we retrieve the inode->i_name of test2 by dump.f2fs, we get
> test1 instead of test2.
> This is because f2fs didn't update the file name during the f2fs_rename.

Er...  Correct me if I'm wrong, but f2fs appears to support link(2) and
if rename(2) creates some problem for dump.f2fs, I would expect an
equivalent link()+unlink() combination to do the same...

Re: [PATCH] f2fs: update file name in the inode block during f2fs_rename

[Al Viro]

On Fri, Jul 19, 2013 at 12:40:47PM +0900, Kim Jaegeuk wrote:
> Hi,
> 
> 2013. 7. 18. ???? 6:22?? "Al Viro" <viro@zeniv.linux.org.uk>???? ????:
> >
> > On Thu, Jul 18, 2013 at 06:11:23PM +0900, Jaegeuk Kim wrote:
> > > The error is reproducible by:
> > >
> > > After this, when we retrieve the inode->i_name of test2 by dump.f2fs,
> we get
> > > test1 instead of test2.
> > > This is because f2fs didn't update the file name during the f2fs_rename.
> >
> > Er...  Correct me if I'm wrong, but f2fs appears to support link(2) and
> > if rename(2) creates some problem for dump.f2fs, I would expect an
> > equivalent link()+unlink() combination to do the same...
> 
> Right. I will check that too.
> Thank you. :)

You do realize that having unlink() hunt for the surviving links would be
both very costly and painful wrt locking, right?

The real question is, what are the warranties for that ->i_name thing?
What should it be while there are multiple links?  Matter of fact,
after looking at the users... What about ->i_pino in the same scenario
(link+unlink instead of rename)?

BTW, while looking at i_pino... Why does get_parent_ino() bother with
igrab/iput?  If you have found an alias, just use parent_ino(dentry)
and be done with that - as it is, you have a race with d_move() there,
so you'd need to reproduce parent_ino() locking anyway (->d_lock on
dentry holds d_move() away and stabilizes ->d_parent->d_inode)

Re: [PATCH] f2fs: update file name in the inode block during f2fs_rename

[Jaegeuk Kim]

Hi Al,

2013-07-19 (금), 08:49 +0100, Al Viro:
> On Fri, Jul 19, 2013 at 12:40:47PM +0900, Kim Jaegeuk wrote:
> > Hi,
> > 
> > 2013. 7. 18. ???? 6:22?? "Al Viro" <viro@zeniv.linux.org.uk>???? ????:
> > >
> > > On Thu, Jul 18, 2013 at 06:11:23PM +0900, Jaegeuk Kim wrote:
> > > > The error is reproducible by:
> > > >
> > > > After this, when we retrieve the inode->i_name of test2 by dump.f2fs,
> > we get
> > > > test1 instead of test2.
> > > > This is because f2fs didn't update the file name during the f2fs_rename.
> > >
> > > Er...  Correct me if I'm wrong, but f2fs appears to support link(2) and
> > > if rename(2) creates some problem for dump.f2fs, I would expect an
> > > equivalent link()+unlink() combination to do the same...
> > 
> > Right. I will check that too.
> > Thank you. :)
> 
> You do realize that having unlink() hunt for the surviving links would be
> both very costly and painful wrt locking, right?

What I meant that I need to check f2fs_sync_file() to deal with the
link() + unlink() combination case.

> 
> The real question is, what are the warranties for that ->i_name thing?
> What should it be while there are multiple links?  Matter of fact,
> after looking at the users... What about ->i_pino in the same scenario
> (link+unlink instead of rename)?

The only usage of both i_name and i_pino is for the roll-forward
recovery. 

Let me give a scenario like this.
1. create "file a"
2. fsync "file a"
  -> At this moment, in order to recover "file a", naive f2fs needs to
conduct costly checkpoint to flush all the dentry blocks.

But, in the roll-forward machinism with a dentry recovery routine,
1. create "file a"
2. fsync "file a"
  -> The f2fs stores i_name as "file a" and its i_pino so that
recover_dentry() can add link again with "file a" under i_pino.

But, there are some creteria like:
1. link_count should be one, and
2. its i_name should be correct.

But, I think i_pino is correctly fixed at f2fs_sync_file() even if there
have been experienced link() + unlink() combinations before.
But, i_name should have to be fixed too not just for f2fs_rename().
This is what I concerned before.

> 
> BTW, while looking at i_pino... Why does get_parent_ino() bother with
> igrab/iput?  If you have found an alias, just use parent_ino(dentry)
> and be done with that - as it is, you have a race with d_move() there,
> so you'd need to reproduce parent_ino() locking anyway (->d_lock on
> dentry holds d_move() away and stabilizes ->d_parent->d_inode)

Oh, I didn't realize that.
I'll fix like that.
Thanks,

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Jaegeuk Kim
Samsung