linux-kernel - Re: [PATCH V1 02/17] ext4: Add the basic function for inline data support.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DACDC44B-F60E-493E-9C15-67EDFCDFD34D@dilger.ca>
Date:	Wed, 26 Oct 2011 16:28:42 -0600
From:	Andreas Dilger <aedilger@...il.com>
To:	Tao Ma <tm@....ma>
Cc:	linux-ext4@...r.kernel.org, tytso@....edu,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 02/17] ext4: Add the basic function for inline data support.

On 2011-10-26, at 8:38 AM, Tao Ma wrote:
> On 10/26/2011 04:36 PM, Andreas Dilger wrote:
>> On 2011-10-26, at 1:34 AM, Tao Ma wrote:
>>> +void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
>>> +			    void *buffer, loff_t pos, unsigned len)
>>> +{
>>> +	header = IHDR(inode, ext4_raw_inode(iloc));
>>> +	entry = (struct ext4_xattr_entry *)((void *)ext4_raw_inode(iloc) +
>>> +					    EXT4_I(inode)->i_inline_off);
>>> +	memcpy((void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs) + pos,
>>> +	       buffer + pos, len);
>>> +}
>>> +
>>> +int ext4_init_inline_data(handle_t *handle, struct inode *inode,
>>> +			  struct ext4_iloc *iloc)
>>> +{
>>> +	size = ext4_get_max_inline_size(inode);
>>> +	value = kzalloc(size, GFP_NOFS);
>>> +	if (!value)
>>> +		return -ENOMEM;
>>> +
>>> +	error = ext4_xattr_ibody_set(handle, inode, &i, &is);
>>> +}
>> 
>> Since file data is changed very rarely, instead of consuming the full
>> xattr space that may not be needed, wouldn't it be better to change
>> ext4_write_inline_data() to just to the ext4_xattr_ibody_set() to save
>> the exact-sized buffer into the xattr?  That will allow other xattrs
>> to be stored in this space as well as the inline data.
> 
> I am just worried about the cpu usage. You know, the xattr values in
> ext4 has to be packed so if we change the content of an inline file
> frequently(say append), the inline xattr value will be removed and added
> frequently which should consume much cpu cycles.  What's more, the other
> xattr values has to be moved also if they are not aligned to the end of
> the inode. I am not sure whether it is good for performance or not.

I'd also guess it isn't the most CPU efficient mechanism, but the main
question is whether this extra CPU usage is even noticeable compared
to the IO time?  Even with the added CPU usage, there is a dramatic
reduction in the IO (no external block to write), so it would always
be a net win to do it that way.

> Another side effect is that we have to write the whole inline data every
> time as a new xattr value replace every time while the current solution
> just needs to memcpy the appended bytes.

What about only storing a new xattr if the file size is increasing, or
when it is truncated to zero?  If the write is <= existing xattr size
then it can use the same mechanism as today (inline overwrite of the
xattr buffer, and update of the xattr checksum).  That avoids overhead
for the case of repeatedly writing a small same-size value into the file.
If some application is appending 1 byte at a time to a file, I think
the CPU overhead in the xattr code is the least of their worries.

The main reason I don't like to consume all of the xattr space right
away is that this will cause OTHER xattrs to immediately be pushed
into the external xattr block (e.g. selinux, security, etc) and then
we will be even worse off than before (file data in inode, xattr in
external block, and added complexity for no benefit).

Cheers, Andreas

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