On Mon, 2012-12-10 at 09:25 -0500, Jeff Layton wrote: > Jian reported that the following sequence would leave "testfile" with > corrupt data: > > # mount localhost:/export /mnt/nfs/ -o vers=3 > # echo abc > /mnt/nfs/testfile; echo def >> /export/testfile; echo ghi >> /mnt/nfs/testfile > # cat -v /export/testfile > abc > ^@^@^@^@ghi > > While there's no locking involved here, the operations are serialized, > so CTO should prevent corruption. > > The first write to the file is fine and writes 4 bytes. The file is then > extended on the server. When it's reopened a GETATTR is issued and the > size change is noticed. This causes NFS_INO_INVALID_DATA to be set on > the file. Because the file is opened for write only, > nfs_want_read_modify_write() returns 0 to nfs_write_begin(). > nfs_updatepage then calls nfs_write_pageuptodate() to see if it should > extend the nfs_page to cover the whole page. NFS_INO_INVALID_DATA is > still set on the file at that point, but that flag is ignored and > nfs_pageuptodate erroneously extends the write to cover the whole page, > with the write done on the server side filled in with zeroes. > > This patch just has that function check for NFS_INO_INVALID_DATA in > addition to NFS_INO_REVAL_PAGECACHE. This fixes the bug, but looking > over the code, I wonder if we might have a similar bug in > nfs_revalidate_size(). The difference between those two flags is very > subtle, so it seems like we ought to be checking for > NFS_INO_INVALID_DATA in most of the places that we look for > NFS_INO_REVAL_PAGECACHE. > > I believe this is regression introduced by commit 8d197a568. The code > did check for NFS_INO_INVALID_DATA prior to that patch. > > Original bug report is here: > > https://bugzilla.redhat.com/show_bug.cgi?id=885743 Hi Jeff, The point of NFS_INO_REVAL_PAGECACHE is to be able to distinguish between situations where the file size or change attribute is in doubt (and so the page cache validity is in doubt), and situations where other attributes are in doubt (such as ACCESS caches, file owner, nlinks,...). This is why we're only checking for NFS_INO_REVAL_PAGECACHE and not NFS_INO_INVALID_DATA in the functions you mention above. So the fix here should be to set NFS_INO_REVAL_PAGECACHE when the change attribute and/or size change is noticed. The only function I can see that appears to get this wrong is nfs_wcc_update_inode(). Does fixing that lay the bug to rest? Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@xxxxxxxxxx www.netapp.com ��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
