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

[Stephen Rothwell]

Hi Jens,

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).

--
Cheers,
Stephen Rothwell s...@canb.auug.org.au

diff --cc drivers/block/loop.c
index e5647690a751,33fde3a39759..000000000000
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@@ -458,36 -416,53 +459,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;
+ 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;

-out:
+ ret = file->f_op->fallocate(file, mode, pos, bio->bi_size);
+ if (unlikely(ret && ret != -EINVAL && ret != -EOPNOTSUPP))
+ ret = -EIO;
return ret;
}

[Jens Axboe]

On 10/31/2013 09:20 PM, Stephen Rothwell wrote:
> Hi Jens,
>
> 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.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Jens Axboe]

On 11/01/2013 02:22 PM, Stephen Rothwell wrote:
> Hi Jens,
>

> On Fri, 01 Nov 2013 09:10:43 -0600 Jens Axboe <ax...@kernel.dk> 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.

[Stephen Rothwell]

Hi Jens,

On Fri, 01 Nov 2013 09:10:43 -0600 Jens Axboe <ax...@kernel.dk> 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 Kleikamp]

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 <ax...@kernel.dk> 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.

Thanks,
Dave

[Jens Axboe]

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

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.

--
Jens Axboe

[Dave Kleikamp]

I could do that.

> 2) I carry the loop updates

The patch is the 17th of the patch set and will break things without
most if not all of the preceding patches which hit a lot of fs code.

> 3) You hand Stephen a merge patch for the resulting merge of the two

I can do that too.

> 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.

This patch set does cross boundaries.

Dave

[Dave Kleikamp]

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.

[Olof Johansson]

Hijacking(?) this thread since it seems relevant:

I noticed the following panic on a chromebox with last night's next.
20131106 shows it as well. I didn't go back further to see. 3.12 runs
fine.

I bisected it down, and unfortunately it points at Stephen's merge commit:

commit 3caa8f38e7eeb56c7d48b0d5c323ffbf4939635d
Merge: 447b374 bb6f7be
Author: Stephen Rothwell <s...@canb.auug.org.au>
Date: Thu Nov 7 14:07:20 2013 +1100

Merge remote-tracking branch 'aio-direct/for-next'

Conflicts:
drivers/block/loop.c
fs/nfs/direct.c
fs/nfs/file.c
include/linux/blk_types.h


... but the branch alone runs fine.

Context to the failure: Userspace is already up and running. ChromeOS
will do ecryptfs and loopback mounts, etc, which is likely where this
is hitting given the process running. It definitely happens during
early userspace setup.

Seems like we might be in for a bumpy ride in 3.13 w.r.t. block if the
breakage we've found this week in -next is any indication.

This seems to be reliably reproducing for me so I can help collect
data if needed, Dave/Jens.

[ 3.373979] EXT4-fs (sda1): mounted filesystem with ordered data
mode. Opts: commit=600
[ 3.385719] EXT4-fs (sda8): mounted filesystem with ordered data
mode. Opts: commit=600
[ 3.475540] bio: create slab <bio-1> at 1
[ 3.483577] EXT4-fs (dm-0): mounted filesystem with ordered data
mode. Opts: discard,commit=600
[ 3.556890] EXT4-fs (sda1): re-mounted. Opts: commit=600,data=ordered
[ 3.636658] ------------[ cut here ]------------
[ 3.641345] kernel BUG at
/mnt/host/source/src/third_party/kernel-next/fs/bio.c:1725!
[ 3.649266] invalid opcode: 0000 [#1] SMP
[ 3.653473] Modules linked in:
[ 3.656610] CPU: 0 PID: 107 Comm: loop0 Tainted: G W
3.12.0-next-20131107 #6
[ 3.664645] Hardware name: SAMSUNG Stumpy, BIOS
Google_Stumpy.2183.0.2012_05_01_1303 05/01/2012
[ 3.673463] task: ffff88010001e250 ti: ffff880074c7e000 task.ti:
ffff880074c7e000
[ 3.681023] RIP: 0010:[<ffffffff8111229d>] [<ffffffff8111229d>]
bio_endio+0x13/0x59
[ 3.688887] RSP: 0018:ffff880074c7fc50 EFLAGS: 00010246
[ 3.694272] RAX: 0000000000000000 RBX: ffff880074cb6120 RCX: 0000000000000000
[ 3.701496] RDX: 00000000fffffffb RSI: fffffffffffffffb RDI: ffff880074c4b000
[ 3.708728] RBP: ffff880074c7fc58 R08: 00000000002df000 R09: 0000000000000200
[ 3.715968] R10: 0000000000000000 R11: ffff880074cb6120 R12: ffffffffffffffff
[ 3.723198] R13: ffffffffffffffea R14: 0000000000010000 R15: 000000000000001f
[ 3.730439] FS: 0000000000000000(0000) GS:ffff880100200000(0000)
knlGS:0000000000000000
[ 3.738590] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3.744383] CR2: 00007fd88c159080 CR3: 000000000180c000 CR4: 00000000000407f0
[ 3.751605] Stack:
[ 3.753657] ffffffff813369b4 ffff880074c7fc98 ffffffff8111f64f
00000000002df000
[ 3.761280] ffff880074cb6120 ffff880074c19000 ffff880074cb6120
0000000000010000
[ 3.768848] 000000000000001f ffff880074c7fd08 ffffffff8111fb00
ffff880075e9a200
[ 3.776419] Call Trace:
[ 3.778900] [<ffffffff813369b4>] ? lo_rw_aio_complete+0x23/0x25
[ 3.785038] [<ffffffff8111f64f>] aio_complete+0x4a/0x1f7
[ 3.790535] [<ffffffff8111fb00>] aio_run_iocb.isra.13+0x304/0x329
[ 3.796781] [<ffffffff8111f041>] ? kzalloc+0xf/0x11
[ 3.801837] [<ffffffff8111fb53>] aio_kernel_submit+0x2e/0x45
[ 3.807658] [<ffffffff81337349>] lo_rw_aio+0x1aa/0x1cf
[ 3.812940] [<ffffffff813378d7>] loop_thread+0x2cf/0x4e7
[ 3.818408] [<ffffffff8104eca6>] ? bit_waitqueue+0x7a/0x7a
[ 3.824009] [<ffffffff81337608>] ? loop_attr_do_show_autoclear+0x1a/0x1a
[ 3.830901] [<ffffffff8104e867>] kthread+0xea/0xf2
[ 3.835857] [<ffffffff8104e77d>] ? flush_kthread_worker+0xba/0xba
[ 3.842085] [<ffffffff814cb8ac>] ret_from_fork+0x7c/0xb0
[ 3.847564] [<ffffffff8104e77d>] ? flush_kthread_worker+0xba/0xba
[ 3.853815] Code: 83 c3 10 41 0f b7 45 58 41 39 c4 7c b7 31 c0 5b
41 5c 41 5d 41 5e 5d c3 66 66 66 66 90 ba fb ff ff ff eb 37 8b 47 44
85 c0 7f 02 <0f> 0b 85 f6 74 07 f0 80 67 10 fe eb 09 48 8b 47 10 a8 01
0f 44
[ 3.874480] RIP [<ffffffff8111229d>] bio_endio+0x13/0x59
[ 3.880004] RSP <ffff880074c7fc50>
[ 3.883602] ---[ end trace ead15c309b799920 ]---
[ 3.888283] Kernel panic - not syncing: Fatal exception
[ 3.893581] Kernel Offset: 0x0 from 0xffffffff81000000 (relocation
range: 0xffffffff80000000-0xffffffff9fffffff)

[Kent Overstreet]

That looks like the bi_remaining BUG_ON() in bio_endio(), probably
related to the loopback driver. I'll start looking at the code soon as I
get into the office, this one should be easy to track down.

[Kent Overstreet]

On Thu, Nov 07, 2013 at 01:20:26PM -0600, Dave Kleikamp wrote:

> Looking back, I obviously rushed the last patch out. This merge patch,
> and the resulting loop.c, fix my problem. My code is working with Jens'
> block tree now.
>
> Jens,
> I ended up replacing a call to bio_iovec_idx() with __bvec_iter_bvec()
> since the former was removed. It's not very elegant, but it works. I'm
> open to suggestions on a cleaner fix, but it can wait until one or both
> of these trees is merged.

No, that's definitely wrong. Read Documentation/block/biovecs.txt - you
need to use either the new bio_iovec() or bio_iovec() iter. I can do the
conversion later today.

[Dave Kleikamp]

Looking back, I obviously rushed the last patch out. This merge patch,
and the resulting loop.c, fix my problem. My code is working with Jens'
block tree now.

Jens,
I ended up replacing a call to bio_iovec_idx() with __bvec_iter_bvec()
since the former was removed. It's not very elegant, but it works. I'm
open to suggestions on a cleaner fix, but it can wait until one or both
of these trees is merged.

I'll be out next week away from internet access, so I'm looking at the
later half of the merge window.

Thanks,
Shaggy

[Dave Kleikamp]

On 11/07/2013 01:25 PM, Kent Overstreet wrote:
> On Thu, Nov 07, 2013 at 01:20:26PM -0600, Dave Kleikamp wrote:
>> On 11/02/2013 03:50 PM, Dave Kleikamp wrote:
>>> On 11/01/2013 03:53 PM, Jens Axboe wrote:
>>
>>>> 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.
>>
>> Looking back, I obviously rushed the last patch out. This merge patch,
>> and the resulting loop.c, fix my problem. My code is working with Jens'
>> block tree now.
>>
>> Jens,
>> I ended up replacing a call to bio_iovec_idx() with __bvec_iter_bvec()
>> since the former was removed. It's not very elegant, but it works. I'm
>> open to suggestions on a cleaner fix, but it can wait until one or both
>> of these trees is merged.
>
> No, that's definitely wrong. Read Documentation/block/biovecs.txt - you
> need to use either the new bio_iovec() or bio_iovec() iter. I can do the
> conversion later today.

I appreciate your help. The patchset requires that the iov_iter
structure can contain either a user-space iovec or a bio_vec, so that
the iov_iter can be passed down transparently into the filesystems. I'll
be happy any way we can get that to work.

Thanks,
Shaggy

[Dave Kleikamp]

On 11/07/2013 01:25 PM, Kent Overstreet wrote:

> On Thu, Nov 07, 2013 at 01:20:26PM -0600, Dave Kleikamp wrote:

>> I ended up replacing a call to bio_iovec_idx() with __bvec_iter_bvec()
>> since the former was removed. It's not very elegant, but it works. I'm
>> open to suggestions on a cleaner fix, but it can wait until one or both
>> of these trees is merged.
>
> No, that's definitely wrong. Read Documentation/block/biovecs.txt - you
> need to use either the new bio_iovec() or bio_iovec() iter. I can do the
> conversion later today.

Stephen,
Can you please drop the aio-direct tree for the time being?

My stuff is not aligning very well with the immutable biovecs and Kent
has a different approach in mind. I'll be working with him on a
replacement that will hopefully be simpler.

Thanks,
Shaggy

[Stephen Rothwell]

Hi all,

On Thu, 07 Nov 2013 18:04:57 -0600 Dave Kleikamp <dave.k...@oracle.com> wrote:
>
> Can you please drop the aio-direct tree for the time being?

OK, I was afraid of this, but, yes, I can drop it. I am not quite sure
what affect this will have on Andrew's tree, though (hopefully not too
much).

This is a bit disappointing. Dave's stuff have been sitting in
linux-next for quite some time (probably too long) so it is not as if
Kent and Jens (should) have been unaware of it.

> My stuff is not aligning very well with the immutable biovecs and Kent
> has a different approach in mind. I'll be working with him on a
> replacement that will hopefully be simpler.

Presumably not before v3.14, right?

[Kent Overstreet]

On Fri, Nov 08, 2013 at 12:53:07PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> On Thu, 07 Nov 2013 18:04:57 -0600 Dave Kleikamp <dave.k...@oracle.com> wrote:
> >
> > Can you please drop the aio-direct tree for the time being?
>
> OK, I was afraid of this, but, yes, I can drop it. I am not quite sure
> what affect this will have on Andrew's tree, though (hopefully not too
> much).
>
> This is a bit disappointing. Dave's stuff have been sitting in
> linux-next for quite some time (probably too long) so it is not as if
> Kent and Jens (should) have been unaware of it.

Yeah, I have to apologize for not noticing sooner. Mea culpa. That said, we
spent some time hashing things out on IRC and I'm pretty excited for the new
plan; Zach Brown also had some good input.

> > My stuff is not aligning very well with the immutable biovecs and Kent
> > has a different approach in mind. I'll be working with him on a
> > replacement that will hopefully be simpler.
>
> Presumably not before v3.14, right?

Yeah, the gist of it is that loopback driver is going to be passing bios
directly to the dio code - my dio rewrite gets us most of the way there, and
after immutable biovecs there's only ~3 (much simpler) patches left that the dio
rewrite depends on.

The remaining issue to deal with is the fact that on writes, the dio code bails
out if it hits an unmapped block (i.e. a hole) and filemap.c finishes it up with
the buffered io code - we need the dio code to handle that in a self contained
fashion. Zach had a (horribly ugly, but definitely workable) idea for that, so
I'm feeling pretty optimistic about this right now.

[Jens Axboe]

On Fri, Nov 08 2013, Stephen Rothwell wrote:
> Hi all,
>
> On Thu, 07 Nov 2013 18:04:57 -0600 Dave Kleikamp <dave.k...@oracle.com> wrote:
> >
> > Can you please drop the aio-direct tree for the time being?
>
> OK, I was afraid of this, but, yes, I can drop it. I am not quite sure
> what affect this will have on Andrew's tree, though (hopefully not too
> much).
>
> This is a bit disappointing. Dave's stuff have been sitting in
> linux-next for quite some time (probably too long) so it is not as if
> Kent and Jens (should) have been unaware of it.

If it doesn't throw merge errors for you, I am not aware of it.

--
Jens Axboe

[Dave Kleikamp]

On 11/07/2013 08:08 PM, Kent Overstreet wrote:
> On Fri, Nov 08, 2013 at 12:53:07PM +1100, Stephen Rothwell wrote:
>> Hi all,
>>
>> On Thu, 07 Nov 2013 18:04:57 -0600 Dave Kleikamp <dave.k...@oracle.com> wrote:
>>>
>>> Can you please drop the aio-direct tree for the time being?
>>
>> OK, I was afraid of this, but, yes, I can drop it. I am not quite sure
>> what affect this will have on Andrew's tree, though (hopefully not too
>> much).
>>
>> This is a bit disappointing. Dave's stuff have been sitting in
>> linux-next for quite some time (probably too long) so it is not as if
>> Kent and Jens (should) have been unaware of it.
>
> Yeah, I have to apologize for not noticing sooner. Mea culpa. That said, we
> spent some time hashing things out on IRC and I'm pretty excited for the new
> plan; Zach Brown also had some good input.

Right. I should have been more proactive in discussing things with the
aio folks as well. After a good discussion with Kent and Zach, I feel
that my stuff will fit in better after Kent's dio rewrite than the
current implementation. We were both working towards very similar goals
and Kent's stuff would probably have ended up replacing a good portion
of my patchset resulting in unnecessary churn in the direct-io path.

[Christoph Hellwig]

Btw, I have to state that I very much disagree with dropping the
direct I/O kernel changes, and I also very much disagree with keeping
the immutable iovecs in.

For the latter I think the immutable iovecs are useful and do want to
see them eventually, but they were merged at the latest possible point
in the merge window and cause breakage all over the tree, so they very
clearly are not ready at this point, and I fear even more breakage if
they do get merged.

The changes for direct I/O from kernel space have been in for a long
time, and they are blocking multiple consumers of the interface from
getting submitted for about a year now. Even if the guts of the
direct-io code will get a write based on another patchset from Kent
that will go on top of the immutable iovec changes we need the
interfaces now and not another year down the road.

[Kent Overstreet]

On Thu, Nov 07, 2013 at 11:33:24PM -0800, Christoph Hellwig wrote:
> The changes for direct I/O from kernel space have been in for a long
> time, and they are blocking multiple consumers of the interface from
> getting submitted for about a year now. Even if the guts of the
> direct-io code will get a write based on another patchset from Kent
> that will go on top of the immutable iovec changes we need the
> interfaces now and not another year down the road.

What else is blocked on this patch series? Honest question.

[Christoph Hellwig]

On Thu, Nov 07, 2013 at 11:39:59PM -0800, Kent Overstreet wrote:
> On Thu, Nov 07, 2013 at 11:33:24PM -0800, Christoph Hellwig wrote:
> > The changes for direct I/O from kernel space have been in for a long
> > time, and they are blocking multiple consumers of the interface from
> > getting submitted for about a year now. Even if the guts of the
> > direct-io code will get a write based on another patchset from Kent
> > that will go on top of the immutable iovec changes we need the
> > interfaces now and not another year down the road.
>
> What else is blocked on this patch series? Honest question.

From me alone:
Support for ecryptfs to not double cache.
AIO support for target_core_file.

To replace code in staging:
Removal of the lustre-specific loop driver.

And I remember others claiming to want to submit bits, too.

[Kent Overstreet]

On Thu, Nov 07, 2013 at 11:44:45PM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2013 at 11:39:59PM -0800, Kent Overstreet wrote:
> > On Thu, Nov 07, 2013 at 11:33:24PM -0800, Christoph Hellwig wrote:
> > > The changes for direct I/O from kernel space have been in for a long
> > > time, and they are blocking multiple consumers of the interface from
> > > getting submitted for about a year now. Even if the guts of the
> > > direct-io code will get a write based on another patchset from Kent
> > > that will go on top of the immutable iovec changes we need the
> > > interfaces now and not another year down the road.
> >
> > What else is blocked on this patch series? Honest question.
>
>
> From me alone:
> Support for ecryptfs to not double cache.
> AIO support for target_core_file.
>
> To replace code in staging:
> Removal of the lustre-specific loop driver.
>
> And I remember others claiming to want to submit bits, too.

So, I don't think the iov_iter stuff is the right approach for solving
the loop issue; it's an ugly hack and after immutable biovecs we're
pretty close to a better solution and some major cleanups too.

I don't know about ecryptfs and AIO for target, though - are there
patches for those you could point me at so I can have a look? I can
believe the iov_iter stuff is necessary for those, but I'd like to
convince myself there isn't a cleaner solution.

Regardless, I don't want to be blocking anyone else's work; if we do
want the iov_iter stuff in now (I'm not arguing one way or the other
till I look at the issues you pointed out) I can write a small patch to
correctly merge with immutable bvecs; I looked at it today and it's
pretty straightforward (if somewhat ugly).

[Christoph Hellwig]

On Thu, Nov 07, 2013 at 11:56:17PM -0800, Kent Overstreet wrote:
> So, I don't think the iov_iter stuff is the right approach for solving
> the loop issue; it's an ugly hack and after immutable biovecs we're
> pretty close to a better solution and some major cleanups too.

All the consumers aren't limited to a block-based filesystem backing,
including loop. So we need a file-ops based approach for in-kernel
dio/aio. If you have a counter proposal please at least describe it.

[Kent Overstreet]

On Fri, Nov 08, 2013 at 12:02:21AM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2013 at 11:56:17PM -0800, Kent Overstreet wrote:
> > So, I don't think the iov_iter stuff is the right approach for solving
> > the loop issue; it's an ugly hack and after immutable biovecs we're
> > pretty close to a better solution and some major cleanups too.
>
> All the consumers aren't limited to a block-based filesystem backing,
> including loop. So we need a file-ops based approach for in-kernel
> dio/aio. If you have a counter proposal please at least describe it.

The core issue isn't whether the IO is going to a block based filesystem
(but thanks for pointing out that that's not necessarily true!) but
whether we want to work with pinned pages or not. If pinned pages are ok
for everything, then bios as a common interface work - likely evolving
them a bit to be more general (it's just bi_bdev and bi_sector that's
actually block specific) - and IMO that would be far preferable to this
abstraction layer.

If OTOH we need a common interface that's also for places where we can't
afford the overhead of pinning user pages - that's a different story,
and maybe we do need all this infrastructure then. That's why I'm asking
about the stuff you meantioned, I'm honestly not sure.

What I'm working towards though is a clean separation between buffered
and direct code paths, so that buffered IO can continue work with iovs
and for O_DIRECT the first thing you do is fill out a bio with pinned
pages and send it down to filesystem code or wherever it's going to go.

That make sense? I can show you more concretely what I'm working on if
you want. Or if I'm full of crap and this is useless for what you guys
want I'm sure you'll let me know :)

[Christoph Hellwig]

On Fri, Nov 08, 2013 at 12:17:37AM -0800, Kent Overstreet wrote:
> The core issue isn't whether the IO is going to a block based filesystem
> (but thanks for pointing out that that's not necessarily true!) but
> whether we want to work with pinned pages or not. If pinned pages are ok
> for everything, then bios as a common interface work - likely evolving
> them a bit to be more general (it's just bi_bdev and bi_sector that's
> actually block specific) - and IMO that would be far preferable to this
> abstraction layer.
>
> If OTOH we need a common interface that's also for places where we can't
> afford the overhead of pinning user pages - that's a different story,
> and maybe we do need all this infrastructure then. That's why I'm asking
> about the stuff you meantioned, I'm honestly not sure.

For both of them we will deal with kernel-allocated pages that are never
mapped to userspace. This is likely to be true for all the consumers
of in-kernel aio/dio as the existing interfaces handle user pages just
fine.

> What I'm working towards though is a clean separation between buffered
> and direct code paths, so that buffered IO can continue work with iovs
> and for O_DIRECT the first thing you do is fill out a bio with pinned
> pages and send it down to filesystem code or wherever it's going to go.

I don't think pushing bios above the fs interface is a good idea. Note
that the iovecs come from userspace for the user pages cases, so there
is little we can do about that, and non-bio based direct I/O
implementations generally work directly at just that level and never
even touch the direct-io.c code.

If you want to redo the ->direct_IO address_space operation and
generic_file_direct_write and the direct I/O side of
generic_file_aio_read (both of which aren't anywhere near as generic as
the name claims) I'm all for it, but it really won't affect the consumer
of the in-kernel aio/dio code.

> That make sense? I can show you more concretely what I'm working on if
> you want. Or if I'm full of crap and this is useless for what you guys
> want I'm sure you'll let me know :)

It sounds interesting, but also a little confusing at this point, at
least from the non-block side of view.

[Kent Overstreet]

On Fri, Nov 08, 2013 at 12:32:51AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 08, 2013 at 12:17:37AM -0800, Kent Overstreet wrote:
> > The core issue isn't whether the IO is going to a block based filesystem
> > (but thanks for pointing out that that's not necessarily true!) but
> > whether we want to work with pinned pages or not. If pinned pages are ok
> > for everything, then bios as a common interface work - likely evolving
> > them a bit to be more general (it's just bi_bdev and bi_sector that's
> > actually block specific) - and IMO that would be far preferable to this
> > abstraction layer.
> >
> > If OTOH we need a common interface that's also for places where we can't
> > afford the overhead of pinning user pages - that's a different story,
> > and maybe we do need all this infrastructure then. That's why I'm asking
> > about the stuff you meantioned, I'm honestly not sure.
>
> For both of them we will deal with kernel-allocated pages that are never
> mapped to userspace. This is likely to be true for all the consumers
> of in-kernel aio/dio as the existing interfaces handle user pages just
> fine.

Ok, that's good to know.

> > What I'm working towards though is a clean separation between buffered
> > and direct code paths, so that buffered IO can continue work with iovs
> > and for O_DIRECT the first thing you do is fill out a bio with pinned
> > pages and send it down to filesystem code or wherever it's going to go.
>
> I don't think pushing bios above the fs interface is a good idea. Note
> that the iovecs come from userspace for the user pages cases, so there
> is little we can do about that, and non-bio based direct I/O
> implementations generally work directly at just that level and never
> even touch the direct-io.c code.

Bios can point to userspage pages just fine (and they do today for DIO
to block devices/block based filesystems today). Don't think of bios as
"block device IOs", just think of them as the equivalent of an iovec +
iov_iter except instead of (potentially userspace) pointers you have
page pointers. That's the core part of what they do (and even if we
don't standardize on bios for that we should standardize on _something_
for that functionality).

Here's the helper function I wrote for my dio rewrite - it should really
take an iov_iter instead of uaddr and len, but user iovec -> bio is the
easy bit:

http://evilpiepirate.org/git/linux-bcache.git/commit/?h=block_stuff&id=4462c03167767c656986afaf981f891705fd5d3b

> If you want to redo the ->direct_IO address_space operation and
> generic_file_direct_write and the direct I/O side of
> generic_file_aio_read (both of which aren't anywhere near as generic as
> the name claims) I'm all for it, but it really won't affect the consumer
> of the in-kernel aio/dio code.

I'm skeptical, but I'm way too tired to make good arguments and this
touches on too much code that I'm less familiar with.

also the flow of control in this code is such a goddamn clusterfuck I
don't even know what to say.

I'll dig more into the ecryptfs and target aio stuff tomorrow though.

> > That make sense? I can show you more concretely what I'm working on if
> > you want. Or if I'm full of crap and this is useless for what you guys
> > want I'm sure you'll let me know :)
>
> It sounds interesting, but also a little confusing at this point, at
> least from the non-block side of view.

Zack, you want to chime in? He was involved in the discussion yesterday,
he might be able to explain this stuff better than I.

[Dave Kleikamp]

On 11/08/2013 01:33 AM, Christoph Hellwig wrote:
> Btw, I have to state that I very much disagree with dropping the
> direct I/O kernel changes, and I also very much disagree with keeping
> the immutable iovecs in.
>
> For the latter I think the immutable iovecs are useful and do want to
> see them eventually, but they were merged at the latest possible point
> in the merge window and cause breakage all over the tree, so they very
> clearly are not ready at this point, and I fear even more breakage if
> they do get merged.
>
> The changes for direct I/O from kernel space have been in for a long
> time, and they are blocking multiple consumers of the interface from
> getting submitted for about a year now. Even if the guts of the
> direct-io code will get a write based on another patchset from Kent
> that will go on top of the immutable iovec changes we need the
> interfaces now and not another year down the road.

Sorry Christoph,
I guess I made a rash decision to pull this from linux-next. I
underestimated the dependencies others had on this.

Stephen,
How about you pick this up again for now? Unfortunately, I'm going to be
on a cruise next week and basically out of touch after today until a
week from Monday. That will give Kent some more time to ponder things
before I ask Linus to merge it.

Thanks,
Sorry for being a hassle.

Shaggy

[Jens Axboe]

On Thu, Nov 07 2013, Christoph Hellwig wrote:
> Btw, I have to state that I very much disagree with dropping the
> direct I/O kernel changes, and I also very much disagree with keeping
> the immutable iovecs in.
>
> For the latter I think the immutable iovecs are useful and do want to
> see them eventually, but they were merged at the latest possible point
> in the merge window and cause breakage all over the tree, so they very
> clearly are not ready at this point, and I fear even more breakage if
> they do get merged.

I agree, I've had this very conversation with Kent as well. The merge of
it has gone a lot worse than I had feared, and the resulting series at
this point is a non-bisectable mess. The fallback plan was to pull it
from the 3.13 tree and shove it into a 3.14 tree with more for-next
simmering.

It is in progress, just takes a while...

--
Jens Axboe

[Jens Axboe]

On Fri, Nov 08 2013, Jens Axboe wrote:
> On Thu, Nov 07 2013, Christoph Hellwig wrote:
> > Btw, I have to state that I very much disagree with dropping the
> > direct I/O kernel changes, and I also very much disagree with keeping
> > the immutable iovecs in.
> >
> > For the latter I think the immutable iovecs are useful and do want to
> > see them eventually, but they were merged at the latest possible point
> > in the merge window and cause breakage all over the tree, so they very
> > clearly are not ready at this point, and I fear even more breakage if
> > they do get merged.
>
> I agree, I've had this very conversation with Kent as well. The merge of
> it has gone a lot worse than I had feared, and the resulting series at
> this point is a non-bisectable mess. The fallback plan was to pull it
> from the 3.13 tree and shove it into a 3.14 tree with more for-next
> simmering.
>
> It is in progress, just takes a while...

And it's done and pushed out. for-3.13/drivers is still missing the
bcache bits, those will get merged back in once they don't depend on the
immutable changes anymore.

Dave, this should make your life easier. And Stephen, if you pull the
new for-next, it should make yours a lot easier as well.

[Zach Brown]

> > > That make sense? I can show you more concretely what I'm working on if
> > > you want. Or if I'm full of crap and this is useless for what you guys
> > > want I'm sure you'll let me know :)
> >
> > It sounds interesting, but also a little confusing at this point, at
> > least from the non-block side of view.
>

> Zach, you want to chime in? He was involved in the discussion yesterday,

> he might be able to explain this stuff better than I.

I can try. I may not do the *best* job because I've been on the
periphery of most of this since I left the proof of concept back at
Oracle :).

The first part is passing in pages instead of mapped addresses. That's
where the iov_iter argument came from. A ham-fisted proof of concept to
try to abstract iterating over any old type of memory. But it's not
*really* abstract because dio magically knows (look for gross
iov_iter_has_iovec() callers) whether the memory is in iovecs or bio
pages when its verifying alignment, pinning or not, etc. In the end
it's little more than syntactic sugar to try and pretend that two
interfaces are one.

For expedience, this iov_iter approach used the loop's bio to store the
pages in the iov_iter rather than translating the bio's pages to a page
array in the iov_iter.

So the first part of what I think Kent is picturing is to take that to
its logical conclusion and have the caller describe the io memory and
offset with a bio instead of explicit address and offset arguments.
This way dio can do nice bio management operations to kick off its
device bios rather than having to clumsily build them from either
incoming pages or mapped user addresses that are hidden in iov_iter.

I'm imagining cutting the current dio up in to two phases. One that
pins user pages and puts them in bios and one that maps those file bios
to device bios and submits them. Then the fop method becomes the second
phase so that loop can call it with its file bios. Call it
->submit_file_bio() instead of ->do_direct_IO(), maybe?

The other part of this series that isn't getting as much attention,
though, is async submission and completion. This patch introduces a
weird in-kernel aio submission interface that adds special cases to aio.
In this new bio world order we could get rid of that complication by
relying on the bio's ->bi_end_io() for completion.

I suppose a high level view of this strategy is to move more towards a
stack where layers have matching inputs and outputs. If both dio and
loop take bios as input and translate them into submitted output bios
then the stacking becomes more natural.

That's the blue sky fantasy anyway. There's a lot of detail being
glossed over. I want to see what the patches look like.

- z

[Stephen Rothwell]

Hi all,

On Fri, 8 Nov 2013 09:15:08 -0700 Jens Axboe <ax...@kernel.dk> wrote:
>
> On Fri, Nov 08 2013, Jens Axboe wrote:
> > On Thu, Nov 07 2013, Christoph Hellwig wrote:
> > > Btw, I have to state that I very much disagree with dropping the
> > > direct I/O kernel changes, and I also very much disagree with keeping
> > > the immutable iovecs in.
> > >
> > > For the latter I think the immutable iovecs are useful and do want to
> > > see them eventually, but they were merged at the latest possible point
> > > in the merge window and cause breakage all over the tree, so they very
> > > clearly are not ready at this point, and I fear even more breakage if
> > > they do get merged.
> >
> > I agree, I've had this very conversation with Kent as well. The merge of
> > it has gone a lot worse than I had feared, and the resulting series at
> > this point is a non-bisectable mess. The fallback plan was to pull it
> > from the 3.13 tree and shove it into a 3.14 tree with more for-next
> > simmering.
> >
> > It is in progress, just takes a while...
>
> And it's done and pushed out. for-3.13/drivers is still missing the
> bcache bits, those will get merged back in once they don't depend on the
> immutable changes anymore.
>
> Dave, this should make your life easier. And Stephen, if you pull the
> new for-next, it should make yours a lot easier as well.

I have added the aio-driect tree back for today.