| Message ID | 1361520691-22665-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- 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
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 > > -- 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
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 >> > -- 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
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 > -- 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
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 >> > -- 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
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 > >> > -- 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
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 > > > -- 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
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. -- 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
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 -- 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
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 -- 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
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 -- 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
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 > -- 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
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 -- 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
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 -- 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
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,
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(-)