ext4: queue conversion after adding to inode's completed IO list

By queuing the io end on the unwritten workqueue before adding it to our inode's list of completed IOs, I think we run the risk of the work getting completed, and the IO freed, before we try to add it to the inode's i_completed_io_list.

It should be safe to add it to the inode's list of completed IOs, and -then- queue it for completion, I think.

Thanks to Dave Chinner for pointing out the race.

Signed-off-by Eric Sandeen <[email protected]>

---

(At least I think this is right; I haven't actually demonstrated a race...)

fs/ext4/inode.c +3 -3

index 0afc8c1..7f56c48 100644
@@ -3804,14 +3804,14 @@ static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
        io_end->flag = EXT4_IO_UNWRITTEN;
        wq = EXT4_SB(io_end->inode->i_sb)->dio_unwritten_wq;
 
-       /* queue the work to convert unwritten extents to written */
-       queue_work(wq, &io_end->work);
-
        /* Add the io_end to per-inode completed aio dio list*/
        ei = EXT4_I(io_end->inode);
        spin_lock_irqsave(&ei->i_completed_io_lock, flags);
        list_add_tail(&io_end->list, &ei->i_completed_io_list);
        spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+
+       /* queue the work to convert unwritten extents to written */
+       queue_work(wq, &io_end->work);
        iocb->private = NULL;

 out:
       if (is_async)

By queuing the io end on the unwritten workqueue before adding it to our inode's list of completed IOs, I think we run the risk of the work getting completed, and the IO freed, before we try to add it to the inode's i_completed_io_list.

It should be safe to add it to the inode's list of completed IOs, and -then- queue it for completion, I think.

Thanks to Dave Chinner for pointing out the race.

Signed-off-by Eric Sandeen <[email protected]>

---

(At least I think this is right; I haven't actually demonstrated a race...)

fs/ext4/inode.c +3 -4

index 0afc8c1..7f56c48 100644
@@ -3804,14 +3804,14 @@ static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
 	io_end->flag = EXT4_IO_UNWRITTEN;
 	wq = EXT4_SB(io_end->inode->i_sb)->dio_unwritten_wq;
 
-	/* queue the work to convert unwritten extents to written */
-	queue_work(wq, &io_end->work);
-
 	/* Add the io_end to per-inode completed aio dio list*/
 	ei = EXT4_I(io_end->inode);
 	spin_lock_irqsave(&ei->i_completed_io_lock, flags);
 	list_add_tail(&io_end->list, &ei->i_completed_io_list);
 	spin_unlock_irqrestore(&ei->i_completed_io_lock, flags);
+
+	/* queue the work to convert unwritten extents to written */
+	queue_work(wq, &io_end->work);
 	iocb->private = NULL;
 out:
 	if (is_async)
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Eric Sandeen

By queuing the io end on the unwritten workqueue before adding it to our inode's list of completed IOs, I think we run the risk of the work getting completed, and the IO freed, before we try to add it to the inode's i_completed_io_list.

It should be safe to add it to the inode's list of completed IOs, and -then- queue it for completion, I think.

Ping? would be good to fix this race, this has jiaying's ack too.