Fix race in xfs_write() between direct and buffered I/O with DMAPI]

fs/xfs/linux-2.6/xfs_lrw.c +4 -1

@@ -707,7 +707,6 @@ start:
 		}
 	}
 
-retry:
 	/* We can write back this queue in page reclaim */
 	current->backing_dev_info = mapping->backing_dev_info;
 

@@ -763,6 +762,17 @@ retry:
 	if (ret == -EIOCBQUEUED && !(ioflags & IO_ISAIO))
 		ret = wait_on_sync_kiocb(iocb);
 
+	isize = i_size_read(inode);
+	if (unlikely(ret < 0 && ret != -EFAULT && *offset > isize))
+		*offset = isize;
+

fs/xfs/linux-2.6/xfs_lrw.c +0 -5

@@ -776,20 +786,7 @@ retry:
 		xfs_ilock(xip, iolock);
 		if (error)
 			goto out_unlock_internal;
-		pos = xip->i_size;
-		ret = 0;
-		goto retry;
-	}
-

Quoted 18 lines · Lachlan McIlroy

fs/xfs/linux-2.6/xfs_lrw.c +4 -1

@@ -707,7 +707,6 @@ start:
 		}
 	}
 
-retry:
 	/* We can write back this queue in page reclaim */
 	current->backing_dev_info = mapping->backing_dev_info;
 

@@ -763,6 +762,17 @@ retry:
 	if (ret == -EIOCBQUEUED && !(ioflags & IO_ISAIO))
 		ret = wait_on_sync_kiocb(iocb);
 
+	isize = i_size_read(inode);
+	if (unlikely(ret < 0 && ret != -EFAULT && *offset > isize))
+		*offset = isize;
+

fs/xfs/linux-2.6/xfs_lrw.c retry: · line 778

Quoted 10 lines

fs/xfs/linux-2.6/xfs_lrw.c +7 -0

+	if (*offset > xip->i_size) {
+		xfs_ilock(xip, XFS_ILOCK_EXCL);
+		if (*offset > xip->i_size)
+			xip->i_size = *offset;
+		xfs_iunlock(xip, XFS_ILOCK_EXCL);
+	}
+
 	if (ret == -ENOSPC &&
 	    DM_EVENT_ENABLED(xip, DM_EVENT_NOSPACE) && !(ioflags & IO_INVIS)) {
 		xfs_iunlock(xip, iolock);

Moving these updates to before the dmapi nospace callout provale doesn't make any changes to the non-dmapi codepath, so good from that perspective. And as you say above it makes sense to have this update before the dmapi callout.

Quoted 9 lines

fs/xfs/linux-2.6/xfs_lrw.c +0 -5

@@ -776,20 +786,7 @@ retry:
 		xfs_ilock(xip, iolock);
 		if (error)
 			goto out_unlock_internal;
-		pos = xip->i_size;
-		ret = 0;
-		goto retry;
-	}
-

fs/xfs/linux-2.6/xfs_lrw.c retry: · line 789

Quoted 10 lines

fs/xfs/linux-2.6/xfs_lrw.c +1 -9

-	isize = i_size_read(inode);
-	if (unlikely(ret < 0 && ret != -EFAULT && *offset > isize))
-		*offset = isize;
-
-	if (*offset > xip->i_size) {
-		xfs_ilock(xip, XFS_ILOCK_EXCL);
-		if (*offset > xip->i_size)
-			xip->i_size = *offset;
-		xfs_iunlock(xip, XFS_ILOCK_EXCL);
+		goto start;

Again all this won't affect non-dmapi operations, so OK with my mainline hat on. Now if we check what start does over the old retry labels:

- calls generic_write_checks. This could and should redo checks based on the new inode size, ok.
- dmapi write even - shouldn't happen because eventsent is non-zero, ok.
- O_DIRECT alignment validation. Superflous, but harmless, ok.
- check for exclusive lock. This is what you said you wanted, and indded due to the lock dropping we need it. But why don't you duplicate this check in the dmapi case below so that we only have to go to start once instead of possibly twice?

- i_new_size update - needed due to the possible i_size changes, ok
- ichgtime - if time passed since the last time we might want to re-updated it, ok
- zero_eof, ok
- setuid clearing, superflous, but harmless.

So the patch looks good to me, but as mention above it might be possible to optimize it a littler more.

Thanks for looking at this Christoph.

I'm not sure what you mean by duplicating the checks. I assume you mean this check:

		if (!need_i_mutex && (mapping->nrpages || pos > xip->i_size)) {
			xfs_iunlock(xip, XFS_ILOCK_EXCL|iolock);
			iolock = XFS_IOLOCK_EXCL;
			need_i_mutex = 1;
			mutex_lock(&inode->i_mutex);
			xfs_ilock(xip, XFS_ILOCK_EXCL|iolock);
			goto start;
		}

This check is there because the default case for direct I/O is to acquire the iolock in shared mode. If we have work to do which requires the iolock to be held exclusive then drop the lock and get it again. Since we dropped the lock then restart.

In the dmapi post-write event it doesn't matter if we have the iolock shared or exclusive - it will be dropped regardless so I don't see how checking the state of the iolock will allow us to avoid a restart.

Do you need more input on this one?

Quoted 1 line · Christoph Hellwig

Do you need more input on this one?

Actually I just might. Based on your last reponse I wasn't sure if you wanted me to make further changes.

Then I got side-tracked wondering
why we even have the 'goto retry' in the dmapi post event - why retry the write if we get ENOSPC when we don't if dmapi is not enabled? Could the write get stuck in an infinite loop?

Quoted 4 lines · Lachlan McIlroy

Quoted 1 line · Christoph Hellwig

Do you need more input on this one?

Actually I just might. Based on your last reponse I wasn't sure if you wanted me to make further changes.

Well, my reponse was that I think we could do it more effecient, but the patch still looks correct to me.

Quoted 5 lines

Then I got side-tracked wondering
why we even have the 'goto retry' in the dmapi post event - why retry the write if we get ENOSPC when we don't if dmapi is not enabled? Could the write get stuck in an infinite loop?

We only retry on ENOSPC if the dmapi nospace even is enabled, or am I missing something?

Quoted 8 lines · Christoph Hellwig

Quoted 4 lines · Lachlan McIlroy

Quoted 1 line · Christoph Hellwig

Do you need more input on this one?

Actually I just might. Based on your last reponse I wasn't sure if you wanted me to make further changes.

Well, my reponse was that I think we could do it more effecient, but the patch still looks correct to me.

Okay great. I'll check it in and we can improve it later when I understand what you meant!

Quoted 6 lines

Quoted 5 lines

Then I got side-tracked wondering
why we even have the 'goto retry' in the dmapi post event - why retry the write if we get ENOSPC when we don't if dmapi is not enabled? Could the write get stuck in an infinite loop?

Quoted 2 lines

We only retry on ENOSPC if the dmapi nospace even is enabled, or am I missing something?

I don't think you're missing anything here. I don't understand how the DMAPI stuff works but I imagined the event was there to indicate that the write failed but what I don't understand is why that justifies a retry. Is there something about DMAPI that needs the write to succeed?

Quoted 18 lines · Lachlan McIlroy

Quoted 7 lines · Christoph Hellwig

Quoted 4 lines · Lachlan McIlroy

Quoted 1 line · Christoph Hellwig

Do you need more input on this one?

Actually I just might. Based on your last reponse I wasn't sure if you wanted me to make further changes.

Well, my reponse was that I think we could do it more effecient, but the patch still looks correct to me.

Okay great. I'll check it in and we can improve it later when I understand what you meant!

Quoted 6 lines

Quoted 4 lines

Then I got side-tracked wondering
why we even have the 'goto retry' in the dmapi post event - why retry the write if we get ENOSPC when we don't if dmapi is not enabled? Could the write get stuck in an infinite loop?

We only retry on ENOSPC if the dmapi nospace even is enabled, or am I missing something?

Quoted 4 lines

I don't think you're missing anything here. I don't understand how the DMAPI stuff works but I imagined the event was there to indicate that the write failed but what I don't understand is why that justifies a retry. Is there something about DMAPI that needs the write to succeed?

yes

http://www.opengroup.org/onlinepubs/9657099/chap3.htm#tagcjh_04_02_04