[PATCH] Btrfs: update inode flags when renaming

[PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

A user reported some weird behaviours,
if we move a file with the noCow flag to a directory without the
noCow flag, the file is now without the flag, but after remount,
we'll find the file's noCow flag comes back.

This is because we missed a proper inode update after inheriting
parent directory's flags,

Reported-by: Marios Titas <redneb8888@gmail.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d9984fa..d2e3352 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 					old_dentry->d_inode,
 					old_dentry->d_name.name,
 					old_dentry->d_name.len);
-		if (!ret)
-			ret = btrfs_update_inode(trans, root, old_inode);
 	}
 	if (ret) {
 		btrfs_abort_transaction(trans, root, ret);
@@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	}
 
 	fixup_inode_flags(new_dir, old_inode);
+	ret = btrfs_update_inode(trans, root, old_inode);
+	if (ret) {
+		btrfs_abort_transaction(trans, root, ret);
+		goto out_fail;
+	}
 
 	ret = btrfs_add_link(trans, new_dir, old_inode,
 			     new_dentry->d_name.name,
-- 
1.7.7.6

Re: [PATCH] Btrfs: update inode flags when renaming

[Marios Titas]

Sorry, but the bug persists even with the above patch.

touch test
chattr +C test
lsattr test
mv test test2
lsattr test2

In the above scenario test2 will not have the C flag.

On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> A user reported some weird behaviours,
> if we move a file with the noCow flag to a directory without the
> noCow flag, the file is now without the flag, but after remount,
> we'll find the file's noCow flag comes back.
>
> This is because we missed a proper inode update after inheriting
> parent directory's flags,
>
> Reported-by: Marios Titas <redneb8888@gmail.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/inode.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d9984fa..d2e3352 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>                                         old_dentry->d_inode,
>                                         old_dentry->d_name.name,
>                                         old_dentry->d_name.len);
> -               if (!ret)
> -                       ret = btrfs_update_inode(trans, root, old_inode);
>         }
>         if (ret) {
>                 btrfs_abort_transaction(trans, root, ret);
> @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>         }
>
>         fixup_inode_flags(new_dir, old_inode);
> +       ret = btrfs_update_inode(trans, root, old_inode);
> +       if (ret) {
> +               btrfs_abort_transaction(trans, root, ret);
> +               goto out_fail;
> +       }
>
>         ret = btrfs_add_link(trans, new_dir, old_inode,
>                              new_dentry->d_name.name,
> --
> 1.7.7.6
>

Re: [PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> Sorry, but the bug persists even with the above patch.
> 
> touch test
> chattr +C test
> lsattr test
> mv test test2
> lsattr test2
> 
> In the above scenario test2 will not have the C flag.

What do you expect?  IMO it's right that test2 does not have the C flag.

This patch ensure that we get the same result after we remount, no more
the C flag coming back :)

thanks,
liubo

> 
> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > A user reported some weird behaviours,
> > if we move a file with the noCow flag to a directory without the
> > noCow flag, the file is now without the flag, but after remount,
> > we'll find the file's noCow flag comes back.
> >
> > This is because we missed a proper inode update after inheriting
> > parent directory's flags,
> >
> > Reported-by: Marios Titas <redneb8888@gmail.com>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/inode.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index d9984fa..d2e3352 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >                                         old_dentry->d_inode,
> >                                         old_dentry->d_name.name,
> >                                         old_dentry->d_name.len);
> > -               if (!ret)
> > -                       ret = btrfs_update_inode(trans, root, old_inode);
> >         }
> >         if (ret) {
> >                 btrfs_abort_transaction(trans, root, ret);
> > @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >         }
> >
> >         fixup_inode_flags(new_dir, old_inode);
> > +       ret = btrfs_update_inode(trans, root, old_inode);
> > +       if (ret) {
> > +               btrfs_abort_transaction(trans, root, ret);
> > +               goto out_fail;
> > +       }
> >
> >         ret = btrfs_add_link(trans, new_dir, old_inode,
> >                              new_dentry->d_name.name,
> > --
> > 1.7.7.6
> >

Re: [PATCH] Btrfs: update inode flags when renaming

[Marios Titas]

You are right, your patch does improve the situation a bit. But it
still does not address the main issue. To illustrate that, consider
the following scenario:

touch test
chattr +C test
head -c 1048576 /dev/zero >> test
mv test test2

now test2 lost the C flag because it was renamed. But the data in
test2 was written before it lost the C flag and so the extents do not
have checksums!
Also try to clone it with BTRFS_IOC_CLONE. It fails as if it had the C flag:

cp --reflink test2 test3

OTOH, if you try to clone over a file with NODATACOW then it works:

touch test3
chattr +C test3
cp --reflink test2 test3

so the file is in an incosistent state: it sometimes behaves as if it
had the NODATACOW flag and sometimes as if it didn't.

Thanks

On Fri, Feb 22, 2013 at 3:40 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
>> Sorry, but the bug persists even with the above patch.
>>
>> touch test
>> chattr +C test
>> lsattr test
>> mv test test2
>> lsattr test2
>>
>> In the above scenario test2 will not have the C flag.
>
> What do you expect?  IMO it's right that test2 does not have the C flag.
>
> This patch ensure that we get the same result after we remount, no more
> the C flag coming back :)
>
> thanks,
> liubo
>
>>
>> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
>> > A user reported some weird behaviours,
>> > if we move a file with the noCow flag to a directory without the
>> > noCow flag, the file is now without the flag, but after remount,
>> > we'll find the file's noCow flag comes back.
>> >
>> > This is because we missed a proper inode update after inheriting
>> > parent directory's flags,
>> >
>> > Reported-by: Marios Titas <redneb8888@gmail.com>
>> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>> > ---
>> >  fs/btrfs/inode.c |    7 +++++--
>> >  1 files changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> > index d9984fa..d2e3352 100644
>> > --- a/fs/btrfs/inode.c
>> > +++ b/fs/btrfs/inode.c
>> > @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>> >                                         old_dentry->d_inode,
>> >                                         old_dentry->d_name.name,
>> >                                         old_dentry->d_name.len);
>> > -               if (!ret)
>> > -                       ret = btrfs_update_inode(trans, root, old_inode);
>> >         }
>> >         if (ret) {
>> >                 btrfs_abort_transaction(trans, root, ret);
>> > @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>> >         }
>> >
>> >         fixup_inode_flags(new_dir, old_inode);
>> > +       ret = btrfs_update_inode(trans, root, old_inode);
>> > +       if (ret) {
>> > +               btrfs_abort_transaction(trans, root, ret);
>> > +               goto out_fail;
>> > +       }
>> >
>> >         ret = btrfs_add_link(trans, new_dir, old_inode,
>> >                              new_dentry->d_name.name,
>> > --
>> > 1.7.7.6
>> >

Re: [PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

On Fri, Feb 22, 2013 at 04:10:37AM -0500, Marios Titas wrote:
> You are right, your patch does improve the situation a bit. But it
> still does not address the main issue. To illustrate that, consider
> the following scenario:

Sorry for so much confusion for users.

Please let me explain the following senario, 

> 
> touch test
> chattr +C test
> head -c 1048576 /dev/zero >> test
> mv test test2
> 
> now test2 lost the C flag because it was renamed. But the data in
> test2 was written before it lost the C flag and so the extents do not
> have checksums!
> Also try to clone it with BTRFS_IOC_CLONE. It fails as if it had the C flag:
> 
> cp --reflink test2 test3

We don't clone a file when the src(test2) file has NODATASUM and the dest(test3)
file does not have NODATASUM, or vice versa.  This ensures our checksum's valid.

Here,
*  test2 does has NODATASUM because test has NODATASUM, while
*  test3 is a new created file, and we're not with '-o nodatasum' or
   '-o nodatacow' mount options or we don't chattr test3,
   so test3 does not have NODATASUM flags set.

So 'cp' ends up 'INVALID'.

> 
> OTOH, if you try to clone over a file with NODATACOW then it works:
> 
> touch test3
> chattr +C test3
> cp --reflink test2 test3

Now test3 is with NODATACOW, so the above 'cp' works.

> 
> so the file is in an incosistent state: it sometimes behaves as if it
> had the NODATACOW flag and sometimes as if it didn't.

The C flag refers to NODATACOW, this NODATACOW is used to tell btrfs
if we write the file's data on COW mode.

So the failure of 'clone' does not equal to the file is NODATACOW.

Feel free to correct me.

thanks,
liubo

> 
> Thanks
> 
> On Fri, Feb 22, 2013 at 3:40 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> >> Sorry, but the bug persists even with the above patch.
> >>
> >> touch test
> >> chattr +C test
> >> lsattr test
> >> mv test test2
> >> lsattr test2
> >>
> >> In the above scenario test2 will not have the C flag.
> >
> > What do you expect?  IMO it's right that test2 does not have the C flag.
> >
> > This patch ensure that we get the same result after we remount, no more
> > the C flag coming back :)
> >
> > thanks,
> > liubo
> >
> >>
> >> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> >> > A user reported some weird behaviours,
> >> > if we move a file with the noCow flag to a directory without the
> >> > noCow flag, the file is now without the flag, but after remount,
> >> > we'll find the file's noCow flag comes back.
> >> >
> >> > This is because we missed a proper inode update after inheriting
> >> > parent directory's flags,
> >> >
> >> > Reported-by: Marios Titas <redneb8888@gmail.com>
> >> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >> > ---
> >> >  fs/btrfs/inode.c |    7 +++++--
> >> >  1 files changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >> > index d9984fa..d2e3352 100644
> >> > --- a/fs/btrfs/inode.c
> >> > +++ b/fs/btrfs/inode.c
> >> > @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >> >                                         old_dentry->d_inode,
> >> >                                         old_dentry->d_name.name,
> >> >                                         old_dentry->d_name.len);
> >> > -               if (!ret)
> >> > -                       ret = btrfs_update_inode(trans, root, old_inode);
> >> >         }
> >> >         if (ret) {
> >> >                 btrfs_abort_transaction(trans, root, ret);
> >> > @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >> >         }
> >> >
> >> >         fixup_inode_flags(new_dir, old_inode);
> >> > +       ret = btrfs_update_inode(trans, root, old_inode);
> >> > +       if (ret) {
> >> > +               btrfs_abort_transaction(trans, root, ret);
> >> > +               goto out_fail;
> >> > +       }
> >> >
> >> >         ret = btrfs_add_link(trans, new_dir, old_inode,
> >> >                              new_dentry->d_name.name,
> >> > --
> >> > 1.7.7.6
> >> >

Re: [PATCH] Btrfs: update inode flags when renaming

[Marios Titas]

On Fri, Feb 22, 2013 at 5:00 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> On Fri, Feb 22, 2013 at 04:10:37AM -0500, Marios Titas wrote:
>> You are right, your patch does improve the situation a bit. But it
>> still does not address the main issue. To illustrate that, consider
>> the following scenario:
>
> Sorry for so much confusion for users.
>
> Please let me explain the following senario,
>
>>
>> touch test
>> chattr +C test
>> head -c 1048576 /dev/zero >> test
>> mv test test2
>>
>> now test2 lost the C flag because it was renamed. But the data in
>> test2 was written before it lost the C flag and so the extents do not
>> have checksums!
>> Also try to clone it with BTRFS_IOC_CLONE. It fails as if it had the C flag:
>>
>> cp --reflink test2 test3
>
> We don't clone a file when the src(test2) file has NODATASUM and the dest(test3)
> file does not have NODATASUM, or vice versa.  This ensures our checksum's valid.
>
> Here,
> *  test2 does has NODATASUM because test has NODATASUM, while
> *  test3 is a new created file, and we're not with '-o nodatasum' or
>    '-o nodatacow' mount options or we don't chattr test3,
>    so test3 does not have NODATASUM flags set.
>
> So 'cp' ends up 'INVALID'.
>
>>
>> OTOH, if you try to clone over a file with NODATACOW then it works:
>>
>> touch test3
>> chattr +C test3
>> cp --reflink test2 test3
>
> Now test3 is with NODATACOW, so the above 'cp' works.
>
>>
>> so the file is in an incosistent state: it sometimes behaves as if it
>> had the NODATACOW flag and sometimes as if it didn't.
>
> The C flag refers to NODATACOW, this NODATACOW is used to tell btrfs
> if we write the file's data on COW mode.
>
> So the failure of 'clone' does not equal to the file is NODATACOW.
>
> Feel free to correct me.

I think that many end users will find all this very confusing. They
will never expect that renaming a file will cause it to suddenly lose
one flag (NODATACOW) while preserving the other (NODATASUM).
Especially since they cannot explicitly control the NODATASUM flag on
a per file basis. I think that renaming a file should preserve all
flags no matter if it's done in the same directory or not. Just like
it preserves permissions, ownership and inode number. So I think
inheriting the flags from the parent on rename is not a good idea
either. Interestingly enough, files don't lose any of the two flags if
instead of renaming you link and then unlink the original.

Re: [PATCH] Btrfs: update inode flags when renaming

[David Sterba]

On Fri, Feb 22, 2013 at 04:19:27PM -0500, Marios Titas wrote:
> I think that many end users will find all this very confusing. They
> will never expect that renaming a file will cause it to suddenly lose
> one flag (NODATACOW) while preserving the other (NODATASUM).
> Especially since they cannot explicitly control the NODATASUM flag on
> a per file basis. I think that renaming a file should preserve all
> flags no matter if it's done in the same directory or not. Just like
> it preserves permissions, ownership and inode number.

I agree. For completeness, the other inherited flags/attributes are
compression statuses. Silently changing them on remove may be wrong in
case the file gains the 'never try to compress' flag after some clever
heuristic (which we do not have yet) decides so.

> So I think
> inheriting the flags from the parent on rename is not a good idea
> either. Interestingly enough, files don't lose any of the two flags if
> instead of renaming you link and then unlink the original.

Link does not take the same codepath as new file or rename. A new
directory entry is created and link count increased.

david

Re: [PATCH] Btrfs: update inode flags when renaming

[Miao Xie]

On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
>> Sorry, but the bug persists even with the above patch.
>>
>> touch test
>> chattr +C test
>> lsattr test
>> mv test test2
>> lsattr test2
>>
>> In the above scenario test2 will not have the C flag.
> 
> What do you expect?  IMO it's right that test2 does not have the C flag.

No, it's not right.
For the users, they expect the C flag is not lost because they just do
a rename operation. but fixup_inode_flags() re-sets the flags by the
parent directory's flag.

I think we should inherit the flags from the parent just when we create
a new file/directory, in the other cases, just give a option to the users.
How do you think about?

Thanks
Miao

> 
> This patch ensure that we get the same result after we remount, no more
> the C flag coming back :)
> 
> thanks,
> liubo
> 
>>
>> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
>>> A user reported some weird behaviours,
>>> if we move a file with the noCow flag to a directory without the
>>> noCow flag, the file is now without the flag, but after remount,
>>> we'll find the file's noCow flag comes back.
>>>
>>> This is because we missed a proper inode update after inheriting
>>> parent directory's flags,
>>>
>>> Reported-by: Marios Titas <redneb8888@gmail.com>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>  fs/btrfs/inode.c |    7 +++++--
>>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index d9984fa..d2e3352 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>                                         old_dentry->d_inode,
>>>                                         old_dentry->d_name.name,
>>>                                         old_dentry->d_name.len);
>>> -               if (!ret)
>>> -                       ret = btrfs_update_inode(trans, root, old_inode);
>>>         }
>>>         if (ret) {
>>>                 btrfs_abort_transaction(trans, root, ret);
>>> @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>         }
>>>
>>>         fixup_inode_flags(new_dir, old_inode);
>>> +       ret = btrfs_update_inode(trans, root, old_inode);
>>> +       if (ret) {
>>> +               btrfs_abort_transaction(trans, root, ret);
>>> +               goto out_fail;
>>> +       }
>>>
>>>         ret = btrfs_add_link(trans, new_dir, old_inode,
>>>                              new_dentry->d_name.name,
>>> --
>>> 1.7.7.6
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Btrfs: update inode flags when renaming

[Marios Titas]

Wouldn't though inheriting create all sorts of problems? For instance
check the example that I give in my other responese [1].

[1] http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg22396.html

On Fri, Feb 22, 2013 at 4:34 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
> On      fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
>> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
>>> Sorry, but the bug persists even with the above patch.
>>>
>>> touch test
>>> chattr +C test
>>> lsattr test
>>> mv test test2
>>> lsattr test2
>>>
>>> In the above scenario test2 will not have the C flag.
>>
>> What do you expect?  IMO it's right that test2 does not have the C flag.
>
> No, it's not right.
> For the users, they expect the C flag is not lost because they just do
> a rename operation. but fixup_inode_flags() re-sets the flags by the
> parent directory's flag.
>
> I think we should inherit the flags from the parent just when we create
> a new file/directory, in the other cases, just give a option to the users.
> How do you think about?
>
> Thanks
> Miao
>
>>
>> This patch ensure that we get the same result after we remount, no more
>> the C flag coming back :)
>>
>> thanks,
>> liubo
>>
>>>
>>> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
>>>> A user reported some weird behaviours,
>>>> if we move a file with the noCow flag to a directory without the
>>>> noCow flag, the file is now without the flag, but after remount,
>>>> we'll find the file's noCow flag comes back.
>>>>
>>>> This is because we missed a proper inode update after inheriting
>>>> parent directory's flags,
>>>>
>>>> Reported-by: Marios Titas <redneb8888@gmail.com>
>>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>>> ---
>>>>  fs/btrfs/inode.c |    7 +++++--
>>>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>>> index d9984fa..d2e3352 100644
>>>> --- a/fs/btrfs/inode.c
>>>> +++ b/fs/btrfs/inode.c
>>>> @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>>                                         old_dentry->d_inode,
>>>>                                         old_dentry->d_name.name,
>>>>                                         old_dentry->d_name.len);
>>>> -               if (!ret)
>>>> -                       ret = btrfs_update_inode(trans, root, old_inode);
>>>>         }
>>>>         if (ret) {
>>>>                 btrfs_abort_transaction(trans, root, ret);
>>>> @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>>         }
>>>>
>>>>         fixup_inode_flags(new_dir, old_inode);
>>>> +       ret = btrfs_update_inode(trans, root, old_inode);
>>>> +       if (ret) {
>>>> +               btrfs_abort_transaction(trans, root, ret);
>>>> +               goto out_fail;
>>>> +       }
>>>>
>>>>         ret = btrfs_add_link(trans, new_dir, old_inode,
>>>>                              new_dentry->d_name.name,
>>>> --
>>>> 1.7.7.6
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
> On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> > On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> >> Sorry, but the bug persists even with the above patch.
> >>
> >> touch test
> >> chattr +C test
> >> lsattr test
> >> mv test test2
> >> lsattr test2
> >>
> >> In the above scenario test2 will not have the C flag.
> > 
> > What do you expect?  IMO it's right that test2 does not have the C flag.
> 
> No, it's not right.
> For the users, they expect the C flag is not lost because they just do
> a rename operation. but fixup_inode_flags() re-sets the flags by the
> parent directory's flag.
> 
> I think we should inherit the flags from the parent just when we create
> a new file/directory, in the other cases, just give a option to the users.
> How do you think about?

Actually what's in my mind is that we just allow files to inherit the parent
flags across different directories.  This can ensure a rename in the same
directory keeps the file's flags.

Sort of this:
fixup_inode_flags(new, old)
{
	if (new == old)
		return;
	...
}

But I don't know if it'll bring new bugs.
Any ideas?

thanks,
liubo

> 
> Thanks
> Miao
> 
> > 
> > This patch ensure that we get the same result after we remount, no more
> > the C flag coming back :)
> > 
> > thanks,
> > liubo
> > 
> >>
> >> On Fri, Feb 22, 2013 at 3:11 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> >>> A user reported some weird behaviours,
> >>> if we move a file with the noCow flag to a directory without the
> >>> noCow flag, the file is now without the flag, but after remount,
> >>> we'll find the file's noCow flag comes back.
> >>>
> >>> This is because we missed a proper inode update after inheriting
> >>> parent directory's flags,
> >>>
> >>> Reported-by: Marios Titas <redneb8888@gmail.com>
> >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>> ---
> >>>  fs/btrfs/inode.c |    7 +++++--
> >>>  1 files changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >>> index d9984fa..d2e3352 100644
> >>> --- a/fs/btrfs/inode.c
> >>> +++ b/fs/btrfs/inode.c
> >>> @@ -7478,8 +7478,6 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>>                                         old_dentry->d_inode,
> >>>                                         old_dentry->d_name.name,
> >>>                                         old_dentry->d_name.len);
> >>> -               if (!ret)
> >>> -                       ret = btrfs_update_inode(trans, root, old_inode);
> >>>         }
> >>>         if (ret) {
> >>>                 btrfs_abort_transaction(trans, root, ret);
> >>> @@ -7514,6 +7512,11 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>>         }
> >>>
> >>>         fixup_inode_flags(new_dir, old_inode);
> >>> +       ret = btrfs_update_inode(trans, root, old_inode);
> >>> +       if (ret) {
> >>> +               btrfs_abort_transaction(trans, root, ret);
> >>> +               goto out_fail;
> >>> +       }
> >>>
> >>>         ret = btrfs_add_link(trans, new_dir, old_inode,
> >>>                              new_dentry->d_name.name,
> >>> --
> >>> 1.7.7.6
> >>>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH] Btrfs: update inode flags when renaming

[David Sterba]

On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
> On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> > On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> >> Sorry, but the bug persists even with the above patch.
> >>
> >> touch test
> >> chattr +C test
> >> lsattr test
> >> mv test test2
> >> lsattr test2
> >>
> >> In the above scenario test2 will not have the C flag.
> > 
> > What do you expect?  IMO it's right that test2 does not have the C flag.
> 
> No, it's not right.
> For the users, they expect the C flag is not lost because they just do
> a rename operation. but fixup_inode_flags() re-sets the flags by the
> parent directory's flag.
> 
> I think we should inherit the flags from the parent just when we create
> a new file/directory, in the other cases, just give a option to the users.
> How do you think about?

I agree with that. The COW status of a file should not be changed at all
when renamed. The typical users are database files and vm images, losing
the NOCOW flag just from moving here and back is quite unexpected.

david

Re: [PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote:
> On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
> > On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> > > On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> > >> Sorry, but the bug persists even with the above patch.
> > >>
> > >> touch test
> > >> chattr +C test
> > >> lsattr test
> > >> mv test test2
> > >> lsattr test2
> > >>
> > >> In the above scenario test2 will not have the C flag.
> > > 
> > > What do you expect?  IMO it's right that test2 does not have the C flag.
> > 
> > No, it's not right.
> > For the users, they expect the C flag is not lost because they just do
> > a rename operation. but fixup_inode_flags() re-sets the flags by the
> > parent directory's flag.
> > 
> > I think we should inherit the flags from the parent just when we create
> > a new file/directory, in the other cases, just give a option to the users.
> > How do you think about?
> 
> I agree with that. The COW status of a file should not be changed at all
> when renamed. The typical users are database files and vm images, losing
> the NOCOW flag just from moving here and back is quite unexpected.
> 
> david

Yeah, I agree to remove this bad 'change in rename', will send a patch to
address it.

thanks,
liubo

Re: [PATCH] Btrfs: update inode flags when renaming

[Miao Xie]

On 	mon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote:
> On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote:
>> On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
>>> On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
>>>> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
>>>>> Sorry, but the bug persists even with the above patch.
>>>>>
>>>>> touch test
>>>>> chattr +C test
>>>>> lsattr test
>>>>> mv test test2
>>>>> lsattr test2
>>>>>
>>>>> In the above scenario test2 will not have the C flag.
>>>>
>>>> What do you expect?  IMO it's right that test2 does not have the C flag.
>>>
>>> No, it's not right.
>>> For the users, they expect the C flag is not lost because they just do
>>> a rename operation. but fixup_inode_flags() re-sets the flags by the
>>> parent directory's flag.
>>>
>>> I think we should inherit the flags from the parent just when we create
>>> a new file/directory, in the other cases, just give a option to the users.
>>> How do you think about?
>>
>> I agree with that. The COW status of a file should not be changed at all
>> when renamed. The typical users are database files and vm images, losing
>> the NOCOW flag just from moving here and back is quite unexpected.
>>
>> david
> 
> Yeah, I agree to remove this bad 'change in rename', will send a patch to
> address it.

I think we can add a mount option, if the option is set, when we move a file 
to a new directory, or create a new file, we will inherit the flags of the parent.
If not set, we inherit the flags only when create a new file.
How do you think about it?

Thanks
Miao

> 
> thanks,
> liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Btrfs: update inode flags when renaming

[Liu Bo]

On Mon, Feb 25, 2013 at 12:23:03PM +0800, Miao Xie wrote:
> On 	mon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote:
> > On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote:
> >> On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
> >>> On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> >>>> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> >>>>> Sorry, but the bug persists even with the above patch.
> >>>>>
> >>>>> touch test
> >>>>> chattr +C test
> >>>>> lsattr test
> >>>>> mv test test2
> >>>>> lsattr test2
> >>>>>
> >>>>> In the above scenario test2 will not have the C flag.
> >>>>
> >>>> What do you expect?  IMO it's right that test2 does not have the C flag.
> >>>
> >>> No, it's not right.
> >>> For the users, they expect the C flag is not lost because they just do
> >>> a rename operation. but fixup_inode_flags() re-sets the flags by the
> >>> parent directory's flag.
> >>>
> >>> I think we should inherit the flags from the parent just when we create
> >>> a new file/directory, in the other cases, just give a option to the users.
> >>> How do you think about?
> >>
> >> I agree with that. The COW status of a file should not be changed at all
> >> when renamed. The typical users are database files and vm images, losing
> >> the NOCOW flag just from moving here and back is quite unexpected.
> >>
> >> david
> > 
> > Yeah, I agree to remove this bad 'change in rename', will send a patch to
> > address it.
> 
> I think we can add a mount option, if the option is set, when we move a file 
> to a new directory, or create a new file, we will inherit the flags of the parent.
> If not set, we inherit the flags only when create a new file.
> How do you think about it?
> 

I'm ok with the option, but...
from some reports on the list, end users are more likely to control, use chattr
files by themselves, inheriting flags via moving a file to a new directory is
indeed not very welcomed.

So for practical use, I assume that it's fairly enough to inherit flags only on
creation?

thanks,
liubo

Re: [PATCH] Btrfs: update inode flags when renaming

[David Sterba]

On Mon, Feb 25, 2013 at 12:23:03PM +0800, Miao Xie wrote:
> On 	mon, 25 Feb 2013 11:50:01 +0800, Liu Bo wrote:
> > On Fri, Feb 22, 2013 at 11:04:40PM +0100, David Sterba wrote:
> >> On Fri, Feb 22, 2013 at 05:34:47PM +0800, Miao Xie wrote:
> >>> On 	fri, 22 Feb 2013 16:40:35 +0800, Liu Bo wrote:
> >>>> On Fri, Feb 22, 2013 at 03:32:50AM -0500, Marios Titas wrote:
> >>>>> Sorry, but the bug persists even with the above patch.
> >>>>>
> >>>>> touch test
> >>>>> chattr +C test
> >>>>> lsattr test
> >>>>> mv test test2
> >>>>> lsattr test2
> >>>>>
> >>>>> In the above scenario test2 will not have the C flag.
> >>>>
> >>>> What do you expect?  IMO it's right that test2 does not have the C flag.
> >>>
> >>> No, it's not right.
> >>> For the users, they expect the C flag is not lost because they just do
> >>> a rename operation. but fixup_inode_flags() re-sets the flags by the
> >>> parent directory's flag.
> >>>
> >>> I think we should inherit the flags from the parent just when we create
> >>> a new file/directory, in the other cases, just give a option to the users.
> >>> How do you think about?
> >>
> >> I agree with that. The COW status of a file should not be changed at all
> >> when renamed. The typical users are database files and vm images, losing
> >> the NOCOW flag just from moving here and back is quite unexpected.
> >>
> >> david
> > 
> > Yeah, I agree to remove this bad 'change in rename', will send a patch to
> > address it.
> 
> I think we can add a mount option, if the option is set, when we move a file 
> to a new directory, or create a new file, we will inherit the flags of the parent.
> If not set, we inherit the flags only when create a new file.
> How do you think about it?

I'd rather see mount options being used for filesystem-wide (or
subvolume-wide) things, and renaming files is a local operation
for which a finer grained control via chattr seems more fit.

david