[PATCH] loop.c: respect bio barrier and sync

[PATCH] loop.c: respect bio barrier and sync

[Constantine Sapuntzakis]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

I believe that the loop block device does not currently respect
barriers or syncs issued by its clients. As a result, I have seen
corrupted log errors when a loopback mounted ext3 file system is
remounted after a hard stop.

The attached patch attempts to fix this problem by respecting the
barrier and sync flags on the I/O request. The sync_file function was
cut-and-paste from the implementation of fsync (I think there's no fd
so I can't call fsync) to allow the patch to be deployed as an updated
module. Is there another function that could be used?

Comments are welcome. I am not on the list so please cc: me on any response.

-Costa

[-- Attachment #2: loop-barrier-patch.diff --]
[-- Type: application/octet-stream, Size: 1432 bytes --]

--- loop.c	2006-05-04 18:48:34.000000000 -0700
+++ loop.c	2006-05-04 18:52:42.000000000 -0700
@@ -467,16 +467,53 @@
 	return ret;
 }
 
+/*
+ * This is best effort. We really wouldn't know what to do with a returned
+ * error. This code is taken from the implementation of fsync.
+ */
+static void sync_file(struct file * file)  
+{
+        struct address_space *mapping;
+
+        if (!file->f_op || !file->f_op->fsync)
+		return;
+
+        mapping = file->f_mapping;
+
+        current->flags |= PF_SYNCWRITE;
+        filemap_fdatawrite(mapping);
+
+        /*
+         * We need to protect against concurrent writers,
+         * which could cause livelocks in fsync_buffers_list
+         */
+        mutex_lock(&mapping->host->i_mutex);
+        file->f_op->fsync(file, file->f_dentry, 1);
+        mutex_unlock(&mapping->host->i_mutex);
+
+        filemap_fdatawait(mapping);
+        current->flags &= ~PF_SYNCWRITE;
+}
+
 static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
 {
 	loff_t pos;
 	int ret;
+	int sync = bio_sync(bio);
+	int barrier = bio_barrier(bio);
+
+	if (barrier)
+		sync_file(lo->lo_backing_file);
 
 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
 	if (bio_rw(bio) == WRITE)
 		ret = lo_send(lo, bio, lo->lo_blocksize, pos);
 	else
 		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
+
+	if (barrier || sync)
+		sync_file(lo->lo_backing_file);
+
 	return ret;
 }
 

Re: [PATCH] loop.c: respect bio barrier and sync

[Jens Axboe]

On Thu, May 04 2006, Constantine Sapuntzakis wrote:
> I believe that the loop block device does not currently respect
> barriers or syncs issued by its clients. As a result, I have seen
> corrupted log errors when a loopback mounted ext3 file system is
> remounted after a hard stop.
> 
> The attached patch attempts to fix this problem by respecting the
> barrier and sync flags on the I/O request. The sync_file function was
> cut-and-paste from the implementation of fsync (I think there's no fd
> so I can't call fsync) to allow the patch to be deployed as an updated
> module. Is there another function that could be used?
> 
> Comments are welcome. I am not on the list so please cc: me on any 
> response..

Please inline your patches, so one can actually comment on them...

- You should handle sync_file() failure. If we don't have !f_op (will
  that ever hit, btw?) or ->fsync(), then fail the barrier with
  -EOPNOTSUPP. For fsync failure, well... You probably want to just
  error the bio with -EIO then.

- bio_sync() doesn't have the semantics you define it to, it is a hint
  to the block layer to start request processing instead of plugging. So
  don't treat it as a barrier, ignore it.

- Does this work for all loop_device types?

-- 
Jens Axboe

Re: [PATCH] loop.c: respect bio barrier and sync

[Constantine Sapuntzakis]

Sorry about the patch as attachments.

> - You should handle sync_file() failure. If we don't have !f_op (will
>   that ever hit, btw?) or ->fsync(), then fail the barrier with
>   -EOPNOTSUPP. For fsync failure, well... You probably want to just
>   error the bio with -EIO then.

Will fix.

> - Does this work for all loop_device types?

What are the other loop_device types?

BTW, should/does an fsync on a block device translate into a disk flush?
I was looking at sync_blockdev and couldn't figure out how that happened.

Thanks,
-Costa