Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [GIT PULL] first round of SCSI updates for the 5.14+ merge window
To:     James Bottomley <James.Bottomley@HansenPartnership.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-scsi <linux-scsi@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Christoph Hellwig <hch@infradead.org>
References: <fc14fbbf0d7c27b7356bc6271ba2a5599d46af58.camel@HansenPartnership.com>
 <CAHk-=wi99u+xj93-pLG0Na7SZmjvWg6n60Pq9Wt9PgO6=exdUA@mail.gmail.com>
 <26c12f13870a2276f41aebfea6e467d576f70860.camel@HansenPartnership.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <18fc05ae-d167-9eee-de16-55223705819d@kernel.dk>
Date:   Thu, 2 Sep 2021 17:32:18 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <26c12f13870a2276f41aebfea6e467d576f70860.camel@HansenPartnership.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/2/21 5:23 PM, James Bottomley wrote:
> On Thu, 2021-09-02 at 15:38 -0700, Linus Torvalds wrote:
>> On Thu, Sep 2, 2021 at 9:50 AM James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>>> We also picked up a non trivial conflict with the already upstream
>>> block tree in st.c
>>
>> Hmm. Resolving that conflict, I just reacted to how the st.c code
>> passes in a NULL gendisk to scsi_ioctl() and then on to
>> blk_execute_rq().
>>
>> Just checking that was fine, and I notice how *many* places do that.
>>
>> Should the blk_execute_rq() function even take that "struct gendisk
>> *bd_disk" argument at all?
>>
>> Maybe the right thing to do would be for the people who care to just
>> set rq->rq_disk before starting the request..
>>
>> But I guess it's traditional, and nobody cares.
> 
> It's certainly traditional, but Christoph has been caring a lot about
> cleaning up our gendisks recently, so he might be interested in seeing
> if he can fix it ...

It's trivially doable, it's more a question of whether it's a good idea
or not... At least when passed in you have to make a conscious decision
on it being NULL. And just like Linus did, it's something you'll notice
and see what other callers are doing.

As I said, I don't feel that strongly about it. Hopefully most cases
that would forget to set it would trigger a NULL defer when they test
their code, and the core does initialize ->rq_disk to NULL. It's less
risky now than earlier, where the request alloc path was less
streamlined.

-- 
Jens Axboe