Re: [PATCH] f2fs: update file name in the inode block duringf2fs_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@xxxxxxxxxxxxxxxxxx>???? ????:
> > >
> > > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Jaegeuk Kim
Samsung

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