don't hold i_sem during O_DIRECT writes to blockdevs

We're moving in the direction of deprecating the raw driver and recommending that applications use O_DIRECT reads and writes against blockdevs.

I don't know how acceptable this will be - there may be operational/admin reasons for preferring the raw driver, and there are certainly application porting issues. So we should continue to support the raw driver well in 2.6.

One weakness which writes to blockdevs have wrt the raw driver is that they are serialised under i_sem. There is no need for this.

This patch changes O_DIRECT writes to blockdevs so that they no longer run under i_sem.

 filemap.c |   71 ++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 44 insertions(+), 27 deletions(-)

@@ -1934,6 +1934,7 @@ generic_file_write(struct file *file, co
 	struct address_space_operations *a_ops = mapping->a_ops;
 	struct inode 	*inode = mapping->host;
 	unsigned long	limit = current->rlim[RLIMIT_FSIZE].rlim_cur;
+	int		isblk = S_ISBLK(inode->i_mode);
 	long		status = 0;
 	loff_t		pos;
 	struct page	*page;

@@ -1953,19 +1954,19 @@ generic_file_write(struct file *file, co
 	pos = *ppos;
 	if (unlikely(pos < 0)) {
 		err = -EINVAL;
-		goto out;
+		goto out_sem;
 	}
 
 	if (unlikely(file->f_error)) {
 		err = file->f_error;
 		file->f_error = 0;
-		goto out;
+		goto out_sem;
 	}
 
 	written = 0;
 
 	/* FIXME: this is for backwards compatibility with 2.4 */
-	if (!S_ISBLK(inode->i_mode) && file->f_flags & O_APPEND)
+	if (!isblk && file->f_flags & O_APPEND)
 		pos = inode->i_size;
 
 	/*

@@ -1975,7 +1976,7 @@ generic_file_write(struct file *file, co
 		if (pos >= limit) {
 			send_sig(SIGXFSZ, current, 0);
 			err = -EFBIG;
-			goto out;
+			goto out_sem;
 		}
 		if (pos > 0xFFFFFFFFULL || count > limit - (u32)pos) {
 			/* send_sig(SIGXFSZ, current, 0); */

@@ -1991,7 +1992,7 @@ generic_file_write(struct file *file, co
 		if (pos >= MAX_NON_LFS) {
 			send_sig(SIGXFSZ, current, 0);
 			err = -EFBIG;
-			goto out;
+			goto out_sem;
 		}
 		if (count > MAX_NON_LFS - (u32)pos) {
 			/* send_sig(SIGXFSZ, current, 0); */

@@ -2006,12 +2007,12 @@ generic_file_write(struct file *file, co
 	 * exceeded without writing data we send a signal and return EFBIG.
 	 * Linus frestrict idea will clean these up nicely..
 	 */
-	if (likely(!S_ISBLK(inode->i_mode))) {
+	if (likely(!isblk)) {
 		if (unlikely(pos >= inode->i_sb->s_maxbytes)) {
 			if (count || pos > inode->i_sb->s_maxbytes) {
 				send_sig(SIGXFSZ, current, 0);
 				err = -EFBIG;
-				goto out;
+				goto out_sem;
 			}
 			/* zero-length writes at ->s_maxbytes are OK */
 		}

@@ -2021,12 +2022,12 @@ generic_file_write(struct file *file, co
 	} else {
 		if (bdev_read_only(inode->i_bdev)) {
 			err = -EPERM;
-			goto out;
+			goto out_sem;
 		}
 		if (pos >= inode->i_size) {
 			if (count || pos > inode->i_size) {
 				err = -ENOSPC;
-				goto out;
+				goto out_sem;
 			}
 		}
 

@@ -2036,7 +2037,7 @@ generic_file_write(struct file *file, co
 
 	err = 0;
 	if (count == 0)
-		goto out;
+		goto out_sem;
 
 	remove_suid(file->f_dentry);
 	time_now = CURRENT_TIME;

@@ -2047,25 +2048,40 @@ generic_file_write(struct file *file, co
 	}
 
 	if (unlikely(file->f_flags & O_DIRECT)) {
-		written = generic_file_direct_IO(WRITE, inode,
+		if (isblk) {
+			up(&inode->i_sem);
+			written = generic_file_direct_IO(WRITE, inode,
 						(char *)buf, pos, count);
-		if (written > 0) {
-			loff_t end = pos + written;
-			if (end > inode->i_size && !S_ISBLK(inode->i_mode)) {
-				inode->i_size = end;
-				mark_inode_dirty(inode);
+			if (written > 0) {
+				if (mapping->nrpages)
+					invalidate_inode_pages2(mapping);
+				*ppos = pos + written;
 			}
-			*ppos = end;
-			if (mapping->nrpages)
-				invalidate_inode_pages2(mapping);
+			err = written;
+			goto out;
+		} else {
+			written = generic_file_direct_IO(WRITE, inode,
+						(char *)buf, pos, count);
+			if (written > 0) {
+				loff_t end = pos + written;
+				if (end > inode->i_size) {
+					inode->i_size = end;
+					mark_inode_dirty(inode);
+				}
+				*ppos = end;
+				/*
+				 * Sync the fs metadata but not the minor inode
+				 * changes and of course not the data as we did
+				 * direct DMA for the IO.
+				 */
+				if (file->f_flags & O_SYNC)
+					status = generic_osync_inode(inode,
+								OSYNC_METADATA);
+				if (mapping->nrpages)
+					invalidate_inode_pages2(mapping);
+			}
+			goto out_status;
 		}
-		/*
-		 * Sync the fs metadata but not the minor inode changes and
-		 * of course not the data as we did direct DMA for the IO.
-		 */
-		if (written >= 0 && file->f_flags & O_SYNC)
-			status = generic_osync_inode(inode, OSYNC_METADATA);
-		goto out_status;
 	}
 
 	do {

@@ -2152,7 +2168,8 @@ generic_file_write(struct file *file, co
 	
 out_status:	
 	err = written ? written : status;
-out:
+out_sem:
 	up(&inode->i_sem);
+out:
 	return err;
 }

.

Please don't make this depenend on S_ISBLK(). There are filesystems (like XFS) that are designed to safely allow concurrent O_DIRECT writes to regular files.

Toe implement this properly we should drop i_sem in the ->direct_IO method of the filesystem/blockdevice. The only question remaining is whether the method has to reqacquire it before returing (and it'll be imediately released again or whether we should change semantics of ->direct_IO to always drop the lock.

The third options would be to never call ->direct_IO with the i_sem held and let filesystems that need it (only ext2 in 2.5 mainline) do synchronization themselves.

I think I prefer option 3, it's the cleanest way of doing it.

A little unrelated, but as you touch the code: what about removing the two existing special cases for S_ISBLK() in generic_file_write()? they're present only to provide the old (pre-LFS) blockdevice semantics on 2.4, we shouldn't keept them around forever..

It could be time to separate out a __generic_file_write() which doesn't take i_sem at all. The ext3 tree was doing that for a while, to permit multipage transactions in journalled data mode.

hm. Are you sure about that? They look fairly useful to me?

-

In fact we we already have that already in the XFS tree (Steve called it do_generic_file_write although I'd really prefer __generic_file_write).

The O_APPEND special casing is certainly very, very ugly - application should use it on block devices at all - if they're screwed when doing it anyway it's their problem.

And I think we can expect reasonable ulimits for root nowdays, although I'm open for discussions on that one.

Forget about this one - I remembered the code wrongly.

This should probably be done unconditionally or not at all.

We've worked very hard on making block devices more "normal" in 2.5.x, and I don't want to start diverging again.

If this is really a scalability issue, I would suggest that people who care look into just getting rid of "i_sem", and replacing it with a read-write semaphore that explicitly protects only "i_size". Then you make reads and non-extending writes take that semaphore for reading, and extending writes and truncates taking it for writing.

[ The "nonextending writes" case is somewhat interesting, a write probably
needs to actually take the semaphore for writing, and then downgrading it to reading after it has checked that it doesn't end up extending the file.

What makes this even more interesting is that depending on the semaphore implementation you can actually split up the "take write lock" into "prepare to take write lock" and "turn it into a read lock" or "confirm write lock", where the "prepare to take write lock" allows existing readers but not new write-lockers, so that if you downgrade to a read lock you never had to synchronize with anybody else who was already reading. ]

I'd much rather do this _right_ than have some ugly blockdev-only hack, since the problem certainly would happen with files too. A lot of people want to do databases on a filesystem, just because it is so much easier to administer.

Linus

I don't know if it is a scalability issue, frankly. It will be for buffered writes, but for writes which wait on IO, the mechanics of the media probably make the benefits small. Conceivably there are some additional merging opportunities, but it's thin.

We can do the rwsem thing, and that would be good. But there may be filesystems which are relying on i_sem to provide protection against concurrent invokations of get_block(create=1), inside i_size.

OK. It'd be nice to get some benchmarks first (say, between O_DIRECT-to-blockdev and the raw driver) to see if it's worth bothering with.

-

We actually want to retain i_sem for directory operations anyway (ie the rw-semaphore would be an addition, not a replacement), so the easiest transition would probably be to move the i_sem thing into the filesystems when the rwsem thing is done (the same way the BKL removal worked), and then let the filesystems make their own decisions on when they need it (and the decision might well be to take it in the "create = 1" case, which is likely to be fairly rare for non-extending writes)

The other alternative is to move this _all_ into the filesystem entirely, and not have "generic_file_write()" take any lock at all. Let the filesystem first take whatever lock it thinks it needs, and then call "generic_file_write()". Filesystems migh choose to just get i_sem unconditionally both for extending and non-extending writes.

That would mean that the filesystems would have to always wrap their use of "generic_file_write()", but a number of them do so anyway because they want to do some other book-keeping. I dunno.

Linus