State of nocow file attribute

State of nocow file attribute

[Lluís Batlle i Rossell]

Hello,

some time ago we discussed on #btrfs that the nocow attribute for files wasn't
working (around 3.3 or 3.4 kernels). That was evident by files fragmenting even
with the attribute set.

Chris mentioned to find a fix quickly for that, and posted some lines of change
into irc. But recently someone mentioned that 3.6-rc looks like still not
respecting nocow for files.

Is there really a fix upstream for that? Do nocow attribute on files work for
anyone already?

Regards,
Lluís.

Re: State of nocow file attribute

[Lluís Batlle i Rossell]

On Wed, Aug 15, 2012 at 08:05:11AM -0500, Kyle Gates wrote:
> > Is there really a fix upstream for that? Do nocow attribute on files work for
> > anyone already?
> 
> IIRC the nocow attribute seems to work if the volume is mounted with "nodatasum"

Shouldn't nocow mean also nodatasum *only for that file*? I find it a bit
useless, to require whole fs nodatasum for per-file nocow to work.

Thank you,
Lluís.

Re: State of nocow file attribute

[Liu Bo]

On 08/15/2012 06:12 PM, Lluís Batlle i Rossell wrote:
> Hello,
> 
> some time ago we discussed on #btrfs that the nocow attribute for files wasn't
> working (around 3.3 or 3.4 kernels). That was evident by files fragmenting even
> with the attribute set.
> 
> Chris mentioned to find a fix quickly for that, and posted some lines of change
> into irc. But recently someone mentioned that 3.6-rc looks like still not
> respecting nocow for files.
> 
> Is there really a fix upstream for that? Do nocow attribute on files work for
> anyone already?
> 

Hi Lluís,

Dave had post a patch to fix it but only enabling NOCOW with zero sized file.

FYI, the patch is http://article.gmane.org/gmane.comp.file-systems.btrfs/17351

With the patch, you don't need to mount with nodatacow any more :)

And why it is only for only zero sized file:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/18046

thanks,
liubo

> Regards,
> Lluís.
> --
> 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
> 

(no subject)

[David Sterba]

On Fri, Aug 17, 2012 at 09:45:20AM +0800, Liu Bo wrote:
> On 08/15/2012 06:12 PM, Lluís Batlle i Rossell wrote:
> > some time ago we discussed on #btrfs that the nocow attribute for files wasn't
> > working (around 3.3 or 3.4 kernels). That was evident by files fragmenting even
> > with the attribute set.
> > 
> > Chris mentioned to find a fix quickly for that, and posted some lines of change
> > into irc. But recently someone mentioned that 3.6-rc looks like still not
> > respecting nocow for files.
> > 
> > Is there really a fix upstream for that? Do nocow attribute on files work for
> > anyone already?
> > 
> 
> Dave had post a patch to fix it but only enabling NOCOW with zero sized file.
> 
> FYI, the patch is http://article.gmane.org/gmane.comp.file-systems.btrfs/17351
> 
> With the patch, you don't need to mount with nodatacow any more :)
> 
> And why it is only for only zero sized file:
> http://permalink.gmane.org/gmane.comp.file-systems.btrfs/18046

the original patch http://permalink.gmane.org/gmane.comp.file-systems.btrfs/18031
did two things, the reasoning why it is not allowed to set nodatasum in
general applies only to the second hunk but this

@@ -139,7 +139,7 @@ void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
 	}

 	if (flags & BTRFS_INODE_NODATACOW)
-		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
+		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW | BTRFS_INODE_NODATASUM;

 	btrfs_update_iflags(inode);
 }
---

is sufficient to create nocow files via a directory with NOCOW attribute
set, and all new files will inherit it (they are automatically
zero-sized so it's safe). This usecase is similar to setting the
COMPRESS attribute on a directory and all new files will inherit the
flag.

If Andrei wants to resend just this particular hunk, I'm giving it my ACK.


david

Re:

[Liu Bo]

On 08/17/2012 10:59 PM, David Sterba wrote:
> On Fri, Aug 17, 2012 at 09:45:20AM +0800, Liu Bo wrote:
>> On 08/15/2012 06:12 PM, Lluís Batlle i Rossell wrote:
>>> some time ago we discussed on #btrfs that the nocow attribute for files wasn't
>>> working (around 3.3 or 3.4 kernels). That was evident by files fragmenting even
>>> with the attribute set.
>>>
>>> Chris mentioned to find a fix quickly for that, and posted some lines of change
>>> into irc. But recently someone mentioned that 3.6-rc looks like still not
>>> respecting nocow for files.
>>>
>>> Is there really a fix upstream for that? Do nocow attribute on files work for
>>> anyone already?
>>>
>>
>> Dave had post a patch to fix it but only enabling NOCOW with zero sized file.
>>
>> FYI, the patch is http://article.gmane.org/gmane.comp.file-systems.btrfs/17351
>>
>> With the patch, you don't need to mount with nodatacow any more :)
>>
>> And why it is only for only zero sized file:
>> http://permalink.gmane.org/gmane.comp.file-systems.btrfs/18046
> 
> the original patch http://permalink.gmane.org/gmane.comp.file-systems.btrfs/18031
> did two things, the reasoning why it is not allowed to set nodatasum in
> general applies only to the second hunk but this
> 
> @@ -139,7 +139,7 @@ void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
>  	}
> 
>  	if (flags & BTRFS_INODE_NODATACOW)
> -		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
> +		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW | BTRFS_INODE_NODATASUM;
> 
>  	btrfs_update_iflags(inode);
>  }
> ---
> 
> is sufficient to create nocow files via a directory with NOCOW attribute
> set, and all new files will inherit it (they are automatically
> zero-sized so it's safe). This usecase is similar to setting the
> COMPRESS attribute on a directory and all new files will inherit the
> flag.
> 
> If Andrei wants to resend just this particular hunk, I'm giving it my ACK.
> 

IMO the following is better, just make use of the original check.  If you agree with this,
I'll send it as a patch :)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6e8f416..d4e58df 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4721,8 +4721,10 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 		if (btrfs_test_opt(root, NODATASUM))
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
 		if (btrfs_test_opt(root, NODATACOW) ||
-		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW))
+		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW)) {
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
+			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
+		}
 	}
 
 	insert_inode_hash(inode);


> 
> 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
> 

Re: State of nocow file attribute

[David Sterba]

On Fri, Aug 17, 2012 at 11:30:14PM +0800, Liu Bo wrote:
> IMO the following is better, just make use of the original check.  If you agree with this,
> I'll send it as a patch :)

I think it's cleaner to keep all flags that get inherited from the
directory -> new file at one place, ie btrfs_inherit_iflags(), than
having them scattered over the code.

> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6e8f416..d4e58df 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4721,8 +4721,10 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
>  		if (btrfs_test_opt(root, NODATASUM))
>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
>  		if (btrfs_test_opt(root, NODATACOW) ||
> -		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW))
> +		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW)) {
>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
> +			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
> +		}
>  	}

And even better, this particular check of dir->flags should be removed
entirely, because it duplicates the equivalent in
btrfs_inherit_iflags().

>  
>  	insert_inode_hash(inode);


david

Re: State of nocow file attribute

[Liu Bo]

On 08/21/2012 10:33 PM, David Sterba wrote:
> On Fri, Aug 17, 2012 at 11:30:14PM +0800, Liu Bo wrote:
>> IMO the following is better, just make use of the original check.  If you agree with this,
>> I'll send it as a patch :)
> 
> I think it's cleaner to keep all flags that get inherited from the
> directory -> new file at one place, ie btrfs_inherit_iflags(), than
> having them scattered over the code.
> 
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 6e8f416..d4e58df 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4721,8 +4721,10 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
>>  		if (btrfs_test_opt(root, NODATASUM))
>>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
>>  		if (btrfs_test_opt(root, NODATACOW) ||
>> -		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW))
>> +		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW)) {
>>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
>> +			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
>> +		}
>>  	}
> 
> And even better, this particular check of dir->flags should be removed
> entirely, because it duplicates the equivalent in
> btrfs_inherit_iflags().
> 

Fine, it's cleaner now.

thanks,
liubo

>>  
>>  	insert_inode_hash(inode);
> 
> 
> 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
>