linux-kernel - Re: [PATCH] squashfs: Migrate from ll_rw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Oct 2019 10:19:54 +0900
From:   Philippe Liard <pliard@...gle.com>
To:     phillip@...ashfs.org.uk, hch@....de
Cc:     linux-kernel@...r.kernel.org, groeck@...omium.org,
        pliard@...gle.com
Subject: Re: [PATCH] squashfs: Migrate from ll_rw_block usage to BIO

> FYI, the mail you quoted never made it to me..
Sorry about that. That was my first time replying on the LKML and I
must have made a mistake when I invoked git send-email.

> On Tue, Oct 29, 2019 at 01:10:13PM +0900, Philippe Liard wrote:
> > > My admittedly limited understanding is that using BIO indirectly
> > > requires buffer_head or an alternative including some
> > > synchronization mechanism at least.
> 
> What access do you need to synchronize?  If you read data into the
> page cache the page lock provides all synchronization needed.  If
> you just read into decrompression buffers there probably is no need
> for synchronization at all as each buffer is only accessed by one
> thread at a time.
My main concern here was waiting for the BIO request to complete but
submit_bio_wait() that you pointed out below should address that.

> > > It's true that the bio_{alloc,add_page,submit}() functions don't
> > > require passing a buffer_head. However because bio_submit() is
> > > asynchronous AFAICT the client needs to use a synchronization
> > > mechanism to wait for and notify the completion of the request
> > > which buffer heads provide. This is achieved respectively by
> > > wait_on_buffer() and {set,clear}_buffer_uptodate().
> 
> submit_bio_wait() is synchronous and takes care of that for you.
Thanks, I should have noticed that.

> > > Another dependency on buffer heads is the fact that
> > > squashfs_read_data() calls into other squashfs functions operating
> > > on buffer heads outside this file. For example
> > > squashfs_decompress() operates on a buffer_head array.
> 
> All the decompressors do is accessing the, and then eventually doing
> a bh_put.  So as a prep patch you can just pass them bio_vecs and
> keep all the buffer head handling in data.c.  Initially that will be
> a little less efficient as it requires two allocations, but as soon
> as you kill off the buffer heads it actually becomes much better.
I will try that, possibily all as a single patch if it looks simple
enough so that there is no need to convert from buffer heads to
bio_vecs. Let me know though if you feel strongly about having this as
two patches.

> 
> > > Given that bio_submit() is asynchronous I'm also not seeing how
> > > the squashfs_bio_request allocation can be removed? There can be
> > > multiple BIO requests in flight each needing to carry some context
> > > used on completion of the request.
> >
> > Christoph, do you still think this could be simplified as you
> > suggested?
> 
> Yes.
Thanks for explaining all of this. I will make the changes you
suggested and will report back with a new patch.