daxfs_write_iter() (daxfs/file.c:356) takes no inode_lock(), and the size update is a read-modify-write on shared state:
if (pos > inode->i_size) {
inode->i_size = pos;
daxfs_update_blocks(inode);
oie = daxfs_overlay_get_inode(info, inode->i_ino);
if (oie) {
WRITE_ONCE(oie->size, cpu_to_le64(pos));
smp_wmb();
}
}Two concurrent extending writes can both read the old i_size, and the smaller one can land last. POSIX also requires writes to a regular file to be atomic with respect to each other, which without the inode lock they are not.
This may well be deliberate. The whole overlay is built on lock-free CAS precisely so that participants do not serialise, and an inode_lock() would only order writers within one kernel anyway, not across the hosts sharing the image, so it would buy local correctness without solving the cross-host case. docs/COHERENCE.md does not currently say either way.
Raising it to get the intent recorded. If it is deliberate, a note in docs/COHERENCE.md stating that concurrent writes to the same file are not serialised and that i_size may be observed stale would close it. If it is not, the size update at least wants a CAS rather than a plain store.
Found during the review in #14.
daxfs_write_iter()(daxfs/file.c:356) takes noinode_lock(), and the size update is a read-modify-write on shared state:Two concurrent extending writes can both read the old
i_size, and the smaller one can land last. POSIX also requires writes to a regular file to be atomic with respect to each other, which without the inode lock they are not.This may well be deliberate. The whole overlay is built on lock-free CAS precisely so that participants do not serialise, and an
inode_lock()would only order writers within one kernel anyway, not across the hosts sharing the image, so it would buy local correctness without solving the cross-host case.docs/COHERENCE.mddoes not currently say either way.Raising it to get the intent recorded. If it is deliberate, a note in
docs/COHERENCE.mdstating that concurrent writes to the same file are not serialised and thati_sizemay be observed stale would close it. If it is not, the size update at least wants a CAS rather than a plain store.Found during the review in #14.