[PATCH] Block - Honour barrier requests in loop driver

[PATCH] Block - Honour barrier requests in loop driver

[Nikanth Karthikesan]

Based on the patch[1] posted by Constantine Sapuntzakis back in May 2006,
and various other similar patches by Jens Axboe, Gerd Hoffmann and Miklos
Szeredi for Suse Kernels, this patch adds barrier support to loop back
devices. I am not sure why this support was not merged earlier.

Jens can you please review this?

Thanks
Nikanth

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0605.0/1172.html

Honour barrier requests in the loop back block device driver.
In case of barrier bios, flush the backing file once before processing the
barrier and once after to guarantee ordering. In case of filesystems that does
not support fsync, barrier bios would be failed with -EOPNOTSUPP.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index bf03455..e805985 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -202,6 +202,35 @@ lo_do_transfer(struct loop_device *lo, int cmd,
 	return lo->transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
 }
 
+static int sync_file(struct file *file)
+{
+	struct address_space *mapping;
+	int ret;
+
+	if (!file->f_op || !file->f_op->fsync)
+		return -EOPNOTSUPP;
+
+	mapping = file->f_mapping;
+
+	ret = filemap_fdatawrite(mapping);
+	if (!ret) {
+		int ret2;
+
+		mutex_lock(&mapping->host->i_mutex);
+		ret = file->f_op->fsync(file, file->f_dentry, 1);
+		mutex_unlock(&mapping->host->i_mutex);
+
+		ret2 = filemap_fdatawait(mapping);
+		if (!ret)
+			ret = ret2;
+	}
+
+	if (unlikely(ret))
+		ret = -EIO;
+
+	return ret;
+}
+
 /**
  * do_lo_send_aops - helper for writing data to a loop device
  *
@@ -472,12 +501,23 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
 {
 	loff_t pos;
 	int ret;
+	int barrier = bio_barrier(bio);
+
+	if (barrier) {
+		ret = sync_file(lo->lo_backing_file);
+		if (unlikely(ret))
+			goto out;
+	}
 
 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
-	if (bio_rw(bio) == WRITE)
+	if (bio_rw(bio) == WRITE) {
 		ret = lo_send(lo, bio, pos);
-	else
+		if (barrier && !ret)
+			ret = sync_file(lo->lo_backing_file);
+	} else
 		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
+
+out:
 	return ret;
 }
 

Re: [PATCH] Block - Honour barrier requests in loop driver

[Christoph Hellwig]

On Tue, Mar 17, 2009 at 02:17:16PM +0530, Nikanth Karthikesan wrote:
> +static int sync_file(struct file *file)
> +{
> +	struct address_space *mapping;
> +	int ret;
> +
> +	if (!file->f_op || !file->f_op->fsync)
> +		return -EOPNOTSUPP;
> +
> +	mapping = file->f_mapping;
> +
> +	ret = filemap_fdatawrite(mapping);
> +	if (!ret) {
> +		int ret2;
> +
> +		mutex_lock(&mapping->host->i_mutex);
> +		ret = file->f_op->fsync(file, file->f_dentry, 1);
> +		mutex_unlock(&mapping->host->i_mutex);
> +
> +		ret2 = filemap_fdatawait(mapping);
> +		if (!ret)
> +			ret = ret2;

Please use vfs_fsync.

> +	int barrier = bio_barrier(bio);
> +
> +	if (barrier) {
> +		ret = sync_file(lo->lo_backing_file);
> +		if (unlikely(ret))
> +			goto out;
> +	}
>  
>  	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
> +	if (bio_rw(bio) == WRITE) {
>  		ret = lo_send(lo, bio, pos);
> +		if (barrier && !ret)
> +			ret = sync_file(lo->lo_backing_file);
> +	} else
>  		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
> +
> +out:
>  	return ret;

We only use barrier requests for reads, which this code relies on for
the second sync, too.  So just move the whole thing into one if block,
ala:


 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
	if (bio_rw(bio) == WRITE) {
		int barrier = bio_barrier(bio);

		if (barrier) {
			ret = sync_file(lo->lo_backing_file);
			if (unlikely(ret))
				goto out;
		}

 		ret = lo_send(lo, bio, pos);
		if (ret)
			goto out;

		if (barrier)
			ret = sync_file(lo->lo_backing_file);
	} else
		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);

out:
 	return ret;

You also should advertise the barrier capability with a queue flag.

Re: [PATCH] Block - Honour barrier requests in loop driver

[Nikanth Karthikesan]

On Tuesday 17 March 2009 14:49:26 Christoph Hellwig wrote:
> On Tue, Mar 17, 2009 at 02:17:16PM +0530, Nikanth Karthikesan wrote:
> > +static int sync_file(struct file *file)
> > +{
> > +	struct address_space *mapping;
> > +	int ret;
> > +
> > +	if (!file->f_op || !file->f_op->fsync)
> > +		return -EOPNOTSUPP;
> > +
> > +	mapping = file->f_mapping;
> > +
> > +	ret = filemap_fdatawrite(mapping);
> > +	if (!ret) {
> > +		int ret2;
> > +
> > +		mutex_lock(&mapping->host->i_mutex);
> > +		ret = file->f_op->fsync(file, file->f_dentry, 1);
> > +		mutex_unlock(&mapping->host->i_mutex);
> > +
> > +		ret2 = filemap_fdatawait(mapping);
> > +		if (!ret)
> > +			ret = ret2;
>
> Please use vfs_fsync.

Ok.

>
> > +	int barrier = bio_barrier(bio);
> > +
> > +	if (barrier) {
> > +		ret = sync_file(lo->lo_backing_file);
> > +		if (unlikely(ret))
> > +			goto out;
> > +	}
> >
> >  	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
> > +	if (bio_rw(bio) == WRITE) {
> >  		ret = lo_send(lo, bio, pos);
> > +		if (barrier && !ret)
> > +			ret = sync_file(lo->lo_backing_file);
> > +	} else
> >  		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
> > +
> > +out:
> >  	return ret;
>
> We only use barrier requests for reads, which this code relies on for
> the second sync, too.  So just move the whole thing into one if block,
> 

you meant barriers only for writes, right? done.

>
> You also should advertise the barrier capability with a queue flag.

Added QUEUE_ORDERED_DRAIN flag.

Here is the updated patch with the above comments from Christoph
incorporated.

Thanks
Nikanth

Honour barrier requests in the loop back block device driver.
In case of barrier bios, flush the backing file once before processing the
barrier and once after to guarantee ordering. In case of filesystems that does
not support fsync, barrier bios would be failed with -EOPNOTSUPP.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index bf03455..8520322 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -474,10 +474,35 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
 	int ret;
 
 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
-	if (bio_rw(bio) == WRITE)
+
+	if (bio_rw(bio) == WRITE) {
+		int barrier = bio_barrier(bio);
+		struct file *file = lo->lo_backing_file;
+
+		if (barrier) {
+			if (!file->f_op || !file->f_op->fsync) {
+				ret = -EOPNOTSUPP;
+				goto out;
+			}
+
+			ret = vfs_fsync(file, file->f_path.dentry, 0);
+			if (unlikely(ret)) {
+				ret = -EIO;
+				goto out;
+			}
+		}
+
 		ret = lo_send(lo, bio, pos);
-	else
+
+		if (barrier && !ret) {
+			ret = vfs_fsync(file, file->f_path.dentry, 0);
+			if (unlikely(ret))
+				ret = -EIO;
+		}
+	} else
 		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
+
+out:
 	return ret;
 }
 
@@ -825,6 +850,10 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	blk_queue_make_request(lo->lo_queue, loop_make_request);
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;
+	if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op &&
+						file->f_op->fsync) {
+		blk_queue_ordered(lo->lo_queue, QUEUE_ORDERED_DRAIN, NULL);
+	}
 
 	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

Re: [PATCH] Block - Honour barrier requests in loop driver

[Christoph Hellwig]

On Tue, Mar 17, 2009 at 05:39:49PM +0530, Nikanth Karthikesan wrote:
> +			if (!file->f_op || !file->f_op->fsync) {
> +				ret = -EOPNOTSUPP;
> +				goto out;
> +			}

file->f_op is never zero, so you can remove the checks for that.
Otherwise looks good.

Re: [PATCH] Block - Honour barrier requests in loop driver

[Nikanth Karthikesan]

On Wednesday 18 March 2009 00:39:05 Christoph Hellwig wrote:
> On Tue, Mar 17, 2009 at 05:39:49PM +0530, Nikanth Karthikesan wrote:
> > +			if (!file->f_op || !file->f_op->fsync) {
> > +				ret = -EOPNOTSUPP;
> > +				goto out;
> > +			}
>
> file->f_op is never zero, so you can remove the checks for that.
> Otherwise looks good.

Thanks for reviewing. I've removed that check.

Jens, Can you merge this?

Thanks
Nikanth

Honour barrier requests in the loop back block device driver.
In case of barrier bios, flush the backing file once before processing the
barrier and once after to guarantee ordering. In case of filesystems that does
not support fsync, barrier bios would be failed with -EOPNOTSUPP.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index bf03455..cac7894 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -474,10 +474,35 @@ static int do_bio_filebacked(struct loop_device *lo, struct bio *bio)
 	int ret;
 
 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
-	if (bio_rw(bio) == WRITE)
+
+	if (bio_rw(bio) == WRITE) {
+		int barrier = bio_barrier(bio);
+		struct file *file = lo->lo_backing_file;
+
+		if (barrier) {
+			if (unlikely(!file->f_op->fsync)) {
+				ret = -EOPNOTSUPP;
+				goto out;
+			}
+
+			ret = vfs_fsync(file, file->f_path.dentry, 0);
+			if (unlikely(ret)) {
+				ret = -EIO;
+				goto out;
+			}
+		}
+
 		ret = lo_send(lo, bio, pos);
-	else
+
+		if (barrier && !ret) {
+			ret = vfs_fsync(file, file->f_path.dentry, 0);
+			if (unlikely(ret))
+				ret = -EIO;
+		}
+	} else
 		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
+
+out:
 	return ret;
 }
 
@@ -825,6 +850,10 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	blk_queue_make_request(lo->lo_queue, loop_make_request);
 	lo->lo_queue->queuedata = lo;
 	lo->lo_queue->unplug_fn = loop_unplug;
+	if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op &&
+						file->f_op->fsync) {
+		blk_queue_ordered(lo->lo_queue, QUEUE_ORDERED_DRAIN, NULL);
+	}
 
 	set_capacity(lo->lo_disk, size);
 	bd_set_size(bdev, size << 9);

Re: [PATCH] Block - Honour barrier requests in loop driver

[Nikanth Karthikesan]

On Wednesday 18 March 2009 10:28:20 Nikanth Karthikesan wrote:
> On Wednesday 18 March 2009 00:39:05 Christoph Hellwig wrote:
> > On Tue, Mar 17, 2009 at 05:39:49PM +0530, Nikanth Karthikesan wrote:
> > > +			if (!file->f_op || !file->f_op->fsync) {
> > > +				ret = -EOPNOTSUPP;
> > > +				goto out;
> > > +			}
> >
> > file->f_op is never zero, so you can remove the checks for that.
> > Otherwise looks good.
>
> Thanks for reviewing. I've removed that check.
>
> Jens, Can you merge this?
>
Hi Jens

Did you get to look at this? Can you merge this?

Thanks
Nikanth

>
> Honour barrier requests in the loop back block device driver.
> In case of barrier bios, flush the backing file once before processing the
> barrier and once after to guarantee ordering. In case of filesystems that
> does not support fsync, barrier bios would be failed with -EOPNOTSUPP.
>
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> ---
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index bf03455..cac7894 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -474,10 +474,35 @@ static int do_bio_filebacked(struct loop_device *lo,
> struct bio *bio) int ret;
>
>  	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
> -	if (bio_rw(bio) == WRITE)
> +
> +	if (bio_rw(bio) == WRITE) {
> +		int barrier = bio_barrier(bio);
> +		struct file *file = lo->lo_backing_file;
> +
> +		if (barrier) {
> +			if (unlikely(!file->f_op->fsync)) {
> +				ret = -EOPNOTSUPP;
> +				goto out;
> +			}
> +
> +			ret = vfs_fsync(file, file->f_path.dentry, 0);
> +			if (unlikely(ret)) {
> +				ret = -EIO;
> +				goto out;
> +			}
> +		}
> +
>  		ret = lo_send(lo, bio, pos);
> -	else
> +
> +		if (barrier && !ret) {
> +			ret = vfs_fsync(file, file->f_path.dentry, 0);
> +			if (unlikely(ret))
> +				ret = -EIO;
> +		}
> +	} else
>  		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
> +
> +out:
>  	return ret;
>  }
>
> @@ -825,6 +850,10 @@ static int loop_set_fd(struct loop_device *lo, fmode_t
> mode, blk_queue_make_request(lo->lo_queue, loop_make_request);
>  	lo->lo_queue->queuedata = lo;
>  	lo->lo_queue->unplug_fn = loop_unplug;
> +	if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op &&
> +						file->f_op->fsync) {
> +		blk_queue_ordered(lo->lo_queue, QUEUE_ORDERED_DRAIN, NULL);
> +	}
>
>  	set_capacity(lo->lo_disk, size);
>  	bd_set_size(bdev, size << 9);

Re: [PATCH] Block - Honour barrier requests in loop driver

[Jens Axboe]

On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> On Wednesday 18 March 2009 10:28:20 Nikanth Karthikesan wrote:
> > On Wednesday 18 March 2009 00:39:05 Christoph Hellwig wrote:
> > > On Tue, Mar 17, 2009 at 05:39:49PM +0530, Nikanth Karthikesan wrote:
> > > > +			if (!file->f_op || !file->f_op->fsync) {
> > > > +				ret = -EOPNOTSUPP;
> > > > +				goto out;
> > > > +			}
> > >
> > > file->f_op is never zero, so you can remove the checks for that.
> > > Otherwise looks good.
> >
> > Thanks for reviewing. I've removed that check.
> >
> > Jens, Can you merge this?
> >
> Hi Jens
> 
> Did you get to look at this? Can you merge this?

Yep, I'll merge it. Thanks!
 

-- 
Jens Axboe

Re: [PATCH] Block - Honour barrier requests in loop driver

[Jens Axboe]

On Tue, Mar 24 2009, Jens Axboe wrote:
> On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> > On Wednesday 18 March 2009 10:28:20 Nikanth Karthikesan wrote:
> > > On Wednesday 18 March 2009 00:39:05 Christoph Hellwig wrote:
> > > > On Tue, Mar 17, 2009 at 05:39:49PM +0530, Nikanth Karthikesan wrote:
> > > > > +			if (!file->f_op || !file->f_op->fsync) {
> > > > > +				ret = -EOPNOTSUPP;
> > > > > +				goto out;
> > > > > +			}
> > > >
> > > > file->f_op is never zero, so you can remove the checks for that.
> > > > Otherwise looks good.
> > >
> > > Thanks for reviewing. I've removed that check.
> > >
> > > Jens, Can you merge this?
> > >
> > Hi Jens
> > 
> > Did you get to look at this? Can you merge this?
> 
> Yep, I'll merge it. Thanks!

Patch is line wrapped, and you still have one more check for f->f_op ==
NULL that you don't need. I have fixed it up, but please fix your
mailer.

-- 
Jens Axboe