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 <[email protected]>
---
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;
}
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.
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 <[email protected]>
---
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);
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.
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 <[email protected]>
---
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);
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 <[email protected]>
>
> ---
>
> 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);
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
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