Re: [PATCH 0/3] Btrfs: stop abusing current->journal_info for direct I/O

[Omar Sandoval]

On Fri, May 11, 2018 at 12:53:36PM +0300, Nikolay Borisov wrote:
> 
> 
> On 11.05.2018 09:30, Omar Sandoval wrote:
> > From: Omar Sandoval 
> > 
> > Hi, everyone,
> > 
> > Btrfs currently abuses current->journal_info in btrfs_direct_IO() in
> > order to pass around some state to get_block() and submit_io(). This
> > hack is ugly and unnecessary because the data we pass around is only
> > used in one call frame. Robbie Ko also pointed out [1] that it could
> > potentially cause a crash if we happen to end up in start_transaction()
> > (e.g., from memory reclaim calling into btrfs_evict_inode(), which can
> > start a transaction). I'm not convinced that Robbie's case can happen in
> > practice since we are using GFP_NOFS for allocations during direct I/O,
> > but either way it's fragile and nasty.
> 
> When I worked initially on btrfs-over-swap I managed to hit a case where
> ext4 stacked on top of btrfs would crash since btrfs will overwrite
> journal_info which was set by ext4. So this change is indeed welcome :)

Yup, that's what I originally made these patches for. Although my latest
idea for swap is to do something along the lines of Darrick's
iomap_swap_activate(): https://patchwork.kernel.org/patch/10376435/,
I'll be getting back to that soon.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Btrfs: stop abusing current->journal_info for direct I/O

[David Sterba]

On Fri, May 11, 2018 at 12:53:36PM +0300, Nikolay Borisov wrote:
> 
> 
> On 11.05.2018 09:30, Omar Sandoval wrote:
> > From: Omar Sandoval 
> > 
> > Hi, everyone,
> > 
> > Btrfs currently abuses current->journal_info in btrfs_direct_IO() in
> > order to pass around some state to get_block() and submit_io(). This
> > hack is ugly and unnecessary because the data we pass around is only
> > used in one call frame. Robbie Ko also pointed out [1] that it could
> > potentially cause a crash if we happen to end up in start_transaction()
> > (e.g., from memory reclaim calling into btrfs_evict_inode(), which can
> > start a transaction). I'm not convinced that Robbie's case can happen in
> > practice since we are using GFP_NOFS for allocations during direct I/O,
> > but either way it's fragile and nasty.
> 
> When I worked initially on btrfs-over-swap I managed to hit a case where
> ext4 stacked on top of btrfs would crash since btrfs will overwrite
> journal_info which was set by ext4. So this change is indeed welcome :)

And also this, https://lkml.org/lkml/2017/12/14/165.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Btrfs: stop abusing current->journal_info for direct I/O

[Nikolay Borisov]

On 11.05.2018 09:30, Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> Hi, everyone,
> 
> Btrfs currently abuses current->journal_info in btrfs_direct_IO() in
> order to pass around some state to get_block() and submit_io(). This
> hack is ugly and unnecessary because the data we pass around is only
> used in one call frame. Robbie Ko also pointed out [1] that it could
> potentially cause a crash if we happen to end up in start_transaction()
> (e.g., from memory reclaim calling into btrfs_evict_inode(), which can
> start a transaction). I'm not convinced that Robbie's case can happen in
> practice since we are using GFP_NOFS for allocations during direct I/O,
> but either way it's fragile and nasty.

When I worked initially on btrfs-over-swap I managed to hit a case where
ext4 stacked on top of btrfs would crash since btrfs will overwrite
journal_info which was set by ext4. So this change is indeed welcome :)

> 
> This series stops using current->journal_info and instead adds some
> extra arguments to the generic direct I/O code so that we can pass
> things around like sane people.
> 
> Based on Linus' master.
> 
> Thanks!
> 
> 1: https://patchwork.kernel.org/patch/10389077/
> 
> Omar Sandoval (3):
>   fs: add initial bh_result->b_private value to __blockdev_direct_IO()
>   fs: add private argument to dio_submit_t
>   Btrfs: stop abusing current->journal_info in btrfs_direct_IO()
> 
>  fs/btrfs/inode.c   | 39 ++-
>  fs/direct-io.c | 12 +++-
>  fs/ext4/inode.c|  6 +++---
>  fs/gfs2/aops.c |  2 +-
>  fs/ocfs2/aops.c|  5 ++---
>  include/linux/fs.h | 10 +-
>  6 files changed, 28 insertions(+), 46 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Btrfs: stop abusing current->journal_info for direct I/O

[David Sterba]

On Thu, May 10, 2018 at 11:30:09PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> Hi, everyone,
> 
> Btrfs currently abuses current->journal_info in btrfs_direct_IO() in
> order to pass around some state to get_block() and submit_io(). This
> hack is ugly and unnecessary because the data we pass around is only
> used in one call frame.

I'd very much like to get rid of the journal_info hack. The changes to
ther filesystems are minimal.

The 3 patches look good to me, you can add my reviewed-by for btrfs
and ack for the rest.  I'm going to do a fstests round too.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html