inode_add/set_bytes and i_blocks, dangerous for small files?

inode_add/set_bytes and i_blocks, dangerous for small files?

[Dominique Martinet]

Hi,

We've been discussing how i_blocks is set because of integrity problems
on 32bit smp and noticed some problems with v9fs as described here[1]; I
could use some help to sort this out.

[1] https://marc.info/?l=linux-fsdevel&m=154834116110451&w=2

Long story short, with 9p and caching activated (e.g. -o cache=fscache),
creating a new file and writing a few bytes will start at 0 and
increment blocks count with inode_add_bytes; so a file with e.g. 200
bytes of data will have i_blocks = 0 and i_bytes = 200.


(added linux-nfs@ in Cc because there is a small, few seconds window
where I can reproduce a similar issue on nfs:
$ echo foo > bar; stat -c %b bar; sleep 3; stat -c %b bar
0
8
with a 4.14.87 knfsd and 4.19.15-300.fc29 client, nfs 4.2, both x86_64,
no export option or explicit client option.
I'm honestly not sure we care about this single-client problem for a few
seconds but figured it's worth reporting)



I believe that from the first byte onwards there should be at least one
block in i_blocks:
 - that's how all the local filesystems I know work, when you write a
single byte you're actualy reserving a few blocks and that is the number
reported with st_blocks ;
 - tools like du show the file as empty ;
 - and most importantly there still are some tools looking at i_blocks
and not doing any read at all if i_blocks is zero (e.g. gnu tar with
some options, see code[2]); I know there's been some discussions around
this for btrfs but I'm not sure how these ended.

[2] http://git.savannah.gnu.org/cgit/tar.git/tree/src/sparse.c#n273


There also is a weird related behaviour since we use inode_add_bytes is
that if you start with an existing file, it'll initially report the
right number of blocks but then the evolution is 100%
client-write-driven e.g. you start with a file with 200 bytes and 8
blocks; then write another 1k and stat will report 10 blocks when the fs
really still only has 8 blocks.
I believe this does not matter as much as this probably doesn't cause
much problem except du confusion.


For what it's worth, cache=none doesn't have the problem because every
stat will send a getattr to the server, so it'd always report the number
of blocks as seen on the server.



Anyway, how is one supposed to use inode_set_bytes/add_bytes for that?
Should we remove our uses of inode_add_bytes ?

Given 9p does not do quota on the client (if required the server can
enforce it), do we care about i_bytes at all?

For cached mounts I'd be open to blatanlty lie and always print the
rounded up value of block based on i_size (e.g. i_size + 511 >> 9 and
i_bytes to 0) thus ignoring what the server report; I don't see much way
around that to have something consistent...


Thanks,
-- 
Dominique Martinet | Asmadeus

NFSv4 i_blocks=0 immediately after creating a file (Was: inode_add/set_bytes and i_blocks, dangerous for small files?)

[Dominique Martinet]

Following up on this for NFS as I made a sneaky bug report hard to
notice, this might be worth having a look:

Dominique Martinet wrote on Fri, Jan 25, 2019:
> [...] There is a small, few seconds window
> where I can reproduce a similar issue on nfs:
> $ echo foo > bar; stat -c %b bar; sleep 3; stat -c %b bar
> 0
> 8
> with a 4.14.87 knfsd and 4.19.15-300.fc29 client, nfs 4.2, both x86_64,
> no export option or explicit client option.

In other words, the file's i_blocks isn't updated for a short while on
nfs v4 which might lead to data loss (see previous email for example
e.g. tar), just create a file and immediately stat to notice.


nfs v3 doesn't exhibit the problem.

I also wasn't able to reproduce on the 4.14.87 machine as a and nfs v4
client so it's likely a semi-recent regression, I could probably bissect
it if really required but I'm sure someone here has some dedicated
hardware for this better than my laptop :)


Feel free to ask if you cannot reproduce and I'll try updating to master
on both client and server just in case.

Thanks,
-- 
Dominique

Re: NFSv4 i_blocks=0 immediately after creating a file (Was: inode_add/set_bytes and i_blocks, dangerous for small files?)

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

On Feb 1, 2019, at 7:28 AM, Dominique Martinet <asmadeus@codewreck.org> wrote:
> 
> Following up on this for NFS as I made a sneaky bug report hard to
> notice, this might be worth having a look:
> 
> Dominique Martinet wrote on Fri, Jan 25, 2019:
>> [...] There is a small, few seconds window
>> where I can reproduce a similar issue on nfs:
>> $ echo foo > bar; stat -c %b bar; sleep 3; stat -c %b bar
>> 0
>> 8
>> with a 4.14.87 knfsd and 4.19.15-300.fc29 client, nfs 4.2, both x86_64,
>> no export option or explicit client option.
> 
> In other words, the file's i_blocks isn't updated for a short while on
> nfs v4 which might lead to data loss (see previous email for example
> e.g. tar), just create a file and immediately stat to notice.
> 
> 
> nfs v3 doesn't exhibit the problem.
> 
> I also wasn't able to reproduce on the 4.14.87 machine as a and nfs v4
> client so it's likely a semi-recent regression, I could probably bissect
> it if really required but I'm sure someone here has some dedicated
> hardware for this better than my laptop :)
> 
> Feel free to ask if you cannot reproduce and I'll try updating to master
> on both client and server just in case.

The "i_blocks == 0" causes problems with some software that thinks the file
has no data and treats it as sparse.  We work around this in ext4 and Lustre
by always reporting i_blocks > 0 for files that have data in them (e.g.
unwritten dirty pages or inline data), see comments nin ext4_file_getattr():

        /*
         * If there is inline data in the inode, the inode will normally not
         * have data blocks allocated (it may have an external xattr block).
         * Report at least one sector for such files, so tools like tar, rsync,
         * others don't incorrectly think the file is completely sparse.
         */
        if (unlikely(ext4_has_inline_data(inode)))
                stat->blocks += (stat->size + 511) >> 9;

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]