On Mon, Sep 09, 2019 at 08:02:13PM +0530, Ritesh Harjani wrote:

On 9/9/19 2:56 PM, Ritesh Harjani wrote:

quoted

quoted

+    ret = iomap_dio_rw(iocb, from, &ext4_iomap_ops,
ext4_dio_write_end_io);
+
+    /*
+     * Unaligned direct AIO must be the only IO in flight or else
+     * any overlapping aligned IO after unaligned IO might result
+     * in data corruption. We also need to wait here in the case
+     * where the inode is being extended so that inode extension
+     * routines in ext4_dio_write_end_io() are covered by the
+     * inode_lock().
+     */
+    if (ret == -EIOCBQUEUED && (unaligned_aio || extend))
+        inode_dio_wait(inode);

So, I looked this more closely into the AIO DIO write extend case
of yours here. AFAICT, this looks good in the sense that it follows
the behavior what we used to have before from __blockdev_direct_IO.
In that it used to wait for AIO DIO writes beyond EOF, but the iomap
framework does not have that. So waiting in case of writes beyond EOF
should be the right thing to do here for ext4 (following the legacy code).

But I would like to confirm the exact race this extend case
is protecting here.
Since writes beyond EOF will require update of inode i_size
(ext4_update_inode_size()) which require us to hold the inode_lock
in exclusive mode, so we must need to wait in extend case here,
even for AIO DIO writes.

Q1. Is above understanding completely correct?

Yes, that's essentially correct.

Q2. Or is there anything else also which it is also protecting which I am
missing?

No, I think that's it...

Do we need to hold inode exclusive lock for ext4_orphan_del() as well?

Yes, I believe so.

Q3. How about XFS then?
(I do see some tricks done with IOLOCK handling in case of ki__pos > i_size
& to zero out the buffer space between old i_size & ki_pos).

But if we talk only about the above case of extending AIO DIO writes beyond
EOF, XFS only takes a shared lock. why?

Looking into XFS code, I see that they have IOLOCK & ILOCK.
So I guess for protecting inode->i_size update they use ILOCK (in
xfs_dio_write_end_io() -> xfs_iomap_write_unwritten()
or ip->i_flags_lock lock in NON-UNWRITTEN case). And for IO part the IOLOCK
is used and hence IOLOCK can be used in shared mode. Is this correct
understanding for XFS?

* David/Christoph/Darrick

I haven't looked at the intricate XFS locking semantics, so I can't really
comment until I've looked at the code to be perfectly honest. Perhaps asking
one of the XFS maintainers could get you the answer you're looking for on
this.

--<M>--