Subject: Re: linux-next: manual merge of the block tree with the tree

From: Dave Kleikamp <[email-masked]>
Date: Sat, 02 Nov 2013 15:50:11 -0500

On 11/01/2013 03:53 PM, Jens Axboe wrote:
> On 11/01/2013 02:41 PM, Dave Kleikamp wrote:
>> On 11/01/2013 03:27 PM, Jens Axboe wrote:
>>> On 11/01/2013 02:22 PM, Stephen Rothwell wrote:
>>>> Hi Jens,
>>>>
>>>> On Fri, 01 Nov 2013 09:10:43 -0600 Jens Axboe <[email-masked]> wrote:
>>>>>
>>>>> On 10/31/2013 09:20 PM, Stephen Rothwell wrote:
>>>>>>
>>>>>> Today's linux-next merge of the block tree got a conflict in
>>>>>> drivers/block/loop.c between commit 2486740b52fd ("loop: use aio to
>>>>>> perform io on the underlying file") from the aio-direct tree and commit
>>>>>> ed2d2f9a8265 ("block: Abstract out bvec iterator") from the block tree.
>>>>>>
>>>>>> I fixed it up (I think - see below - I have also attached the final
>>>>>> resulting file) and can carry the fix as necessary (no action is
>>>>>> required).
>>>>>>
>>>>>
>>>>> What tree is this from? It'd be a lot more convenient to fold that loop
>>>>> patch into my tree, especially since the block tree in linux-next failed
>>>>> after this merge.
>>>>
>>>> I can only agree with you.  It is from the aio-direct tree (probably
>>>> misnamed by me) (git://github.com/kleikamp/linux-shaggy.git#for-next) run
>>>> by Dave Kleikamp.
>>>
>>> Dave, input requested.
>>>
>>> In any case, I would suggest dropping the aio-direct tree instead of the
>>> entire block tree for coverage purposes, if merge or build failures
>>> happen because of it.
>>
>> I've had these patches in linux-next since August, and I'd really like
>> to push them in the 3.13 merge window.
>>
>> Are there other problems besides this merge issue? I'll take a closer
>> look at Stephen's merge patch and see if I find any other issues, but I
>> really don't want to pull these patches out of linux-next now.
> 
> I'm not saying that the patches should be dropped or not go into 3.13.
> What I'm saying is that if the choice is between having the bio and
> blk-mq stuff in linux-next or an addon to the loop driver, the decision
> should be quite clear.
> 
> So we've three immediate options:
> 
> 1) You base it on top of the block tree
> 2) I carry the loop updates
> 3) You hand Stephen a merge patch for the resulting merge of the two

Attached is a merge patch and the merged loop.c. I'm having problems
with the loop driver with both the block and my tree. I'll continue to
look at that, but everything should build cleanly with this.

> It's one of the problems with too-many-tree, imho. You end up with
> dependencies that could have been solved if the work had been applied in
> the right upstream tree. Sometimes that's not even enough though, if you
> end up crossing boundaries.
> 
diff --cc drivers/block/loop.c
index abe147a,33fde3a..0000000
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@@ -219,48 -218,6 +219,55 @@@ lo_do_transfer(struct loop_device *lo, 
  	return lo->transfer(lo, cmd, rpage, roffs, lpage, loffs, size, rblock);
  }
  
 +#ifdef CONFIG_AIO
 +static void lo_rw_aio_complete(u64 data, long res)
 +{
 +	struct bio *bio = (struct bio *)(uintptr_t)data;
 +
 +	if (res > 0)
 +		res = 0;
 +	else if (res < 0)
 +		res = -EIO;
 +
 +	bio_endio(bio, res);
 +}
 +
 +static int lo_rw_aio(struct loop_device *lo, struct bio *bio)
 +{
 +	struct file *file = lo->lo_backing_file;
 +	struct kiocb *iocb;
 +	unsigned int op;
- 	struct iov_iter iter;
- 	struct bio_vec *bvec;
++	struct iov_iter iov_iter;
++	struct bvec_iter iter;
++	struct bio_vec bvec;
 +	size_t nr_segs;
- 	loff_t pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
++	loff_t pos = ((loff_t) bio->bi_iter.bi_sector << 9) + lo->lo_offset;
++	int rc;
 +
 +	iocb = aio_kernel_alloc(GFP_NOIO);
 +	if (!iocb)
 +		return -ENOMEM;
 +
 +	if (bio_rw(bio) & WRITE)
 +		op = IOCB_CMD_WRITE_ITER;
 +	else
 +		op = IOCB_CMD_READ_ITER;
 +
- 	bvec = bio_iovec_idx(bio, bio->bi_idx);
- 	nr_segs = bio_segments(bio);
- 	iov_iter_init_bvec(&iter, bvec, nr_segs, bvec_length(bvec, nr_segs), 0);
- 	aio_kernel_init_rw(iocb, file, iov_iter_count(&iter), pos);
- 	aio_kernel_init_callback(iocb, lo_rw_aio_complete, (u64)(uintptr_t)bio);
- 
- 	return aio_kernel_submit(iocb, op, &iter);
++	bio_for_each_segment(bvec, bio, iter) {
++		nr_segs = bio_segments(bio);
++		iov_iter_init_bvec(&iov_iter, &bvec, nr_segs,
++				   bvec_length(&bvec, nr_segs), 0);
++		aio_kernel_init_rw(iocb, file, iov_iter_count(&iov_iter), pos);
++		aio_kernel_init_callback(iocb, lo_rw_aio_complete,
++					 (u64)(uintptr_t)bio);
++		rc = aio_kernel_submit(iocb, op, &iov_iter);
++		if (rc)
++			break;
++	}
++	return rc;
 +}
 +#endif /* CONFIG_AIO */
 +
  /**
   * __do_lo_send_write - helper for writing data to a loop device
   *
@@@ -458,36 -416,53 +466,36 @@@ static int do_bio_filebacked(struct loo
  	loff_t pos;
  	int ret;
  
- 	pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
+ 	pos = ((loff_t) bio->bi_iter.bi_sector << 9) + lo->lo_offset;
  
  	if (bio_rw(bio) == WRITE) {
 -		struct file *file = lo->lo_backing_file;
 +		ret = lo_send(lo, bio, pos);
 +	} else
 +		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
  
 -		if (bio->bi_rw & REQ_FLUSH) {
 -			ret = vfs_fsync(file, 0);
 -			if (unlikely(ret && ret != -EINVAL)) {
 -				ret = -EIO;
 -				goto out;
 -			}
 -		}
 +	return ret;
 +}
  
 -		/*
 -		 * We use punch hole to reclaim the free space used by the
 -		 * image a.k.a. discard. However we do not support discard if
 -		 * encryption is enabled, because it may give an attacker
 -		 * useful information.
 -		 */
 -		if (bio->bi_rw & REQ_DISCARD) {
 -			struct file *file = lo->lo_backing_file;
 -			int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
 -
 -			if ((!file->f_op->fallocate) ||
 -			    lo->lo_encrypt_key_size) {
 -				ret = -EOPNOTSUPP;
 -				goto out;
 -			}
 -			ret = file->f_op->fallocate(file, mode, pos,
 -						    bio->bi_iter.bi_size);
 -			if (unlikely(ret && ret != -EINVAL &&
 -				     ret != -EOPNOTSUPP))
 -				ret = -EIO;
 -			goto out;
 -		}
 +static int lo_discard(struct loop_device *lo, struct bio *bio)
 +{
 +	struct file *file = lo->lo_backing_file;
 +	int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
- 	loff_t pos = ((loff_t) bio->bi_sector << 9) + lo->lo_offset;
++	loff_t pos = ((loff_t) bio->bi_iter.bi_sector << 9) + lo->lo_offset;
 +	int ret;
  
 -		ret = lo_send(lo, bio, pos);
 +	/*
 +	 * We use punch hole to reclaim the free space used by the
 +	 * image a.k.a. discard. However we do not support discard if
 +	 * encryption is enabled, because it may give an attacker
 +	 * useful information.
 +	 */
  
 -		if ((bio->bi_rw & REQ_FUA) && !ret) {
 -			ret = vfs_fsync(file, 0);
 -			if (unlikely(ret && ret != -EINVAL))
 -				ret = -EIO;
 -		}
 -	} else
 -		ret = lo_receive(lo, bio, lo->lo_blocksize, pos);
 +	if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size)
 +		return -EOPNOTSUPP;
  
- 	ret = file->f_op->fallocate(file, mode, pos, bio->bi_size);
 -out:
++	ret = file->f_op->fallocate(file, mode, pos, bio->bi_iter.bi_size);
 +	if (unlikely(ret && ret != -EINVAL && ret != -EOPNOTSUPP))
 +		ret = -EIO;
  	return ret;
  }