Re: [PATCH v2] md: don't create mddev in md_open

["heming.zhao@suse.com" <heming.zhao@suse.com>]

On 4/3/21 12:01 AM, Song Liu wrote:
> On Fri, Apr 2, 2021 at 1:58 AM heming.zhao@suse.com
> <heming.zhao@suse.com> wrote:
>>
>> On 4/2/21 1:58 PM, Song Liu wrote:
>>> On Thu, Apr 1, 2021 at 6:03 AM heming.zhao@suse.com
>>> <heming.zhao@suse.com> wrote:
>>>>
>>>> On 4/1/21 2:17 PM, Christoph Hellwig wrote:
>>>>> On Thu, Apr 01, 2021 at 09:34:56AM +0800, Zhao Heming wrote:
>>>>>> commit d3374825ce57 ("md: make devices disappear when they are no longer
>>>>>> needed.") introduced protection between mddev creating & removing. The
>>>>>> md_open shouldn't create mddev when all_mddevs list doesn't contain
>>>>>> mddev. With currently code logic, there will be very easy to trigger
>>>>>> soft lockup in non-preempt env.
>>>>>
>>>>> As mention below, please don't make this even more of a mess than it
>>>>> needs to.  We can just pick the two patches doing this from the series
>>>>> I sent:
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> I already got your meaning on previously email.
>>>> I sent v2 patch for Song's review comment. My patch is bugfix, it may need
>>>> to back port into branch maintenance.
>>>>
>>>> Your attachment patch files is partly my patch.
>>>> The left part is in md_open (response [PATCH 01/15] md: remove the code to flush
>>>> an old instance in md_open)
>>>> I still think you directly use bdev->bd_disk->private_data as mddev pointer is not safe.
>>>>
>>>
>>> Hi Christoph and Heming,
>>>
>>> Trying to understand the whole picture here. Please let me know if I
>>> misunderstood anything.
>>>
>>> IIUC, the primary goal of Christoph's set is to get rid of the
>>> ERESTARTSYS hack from md,
>>> and eventually move bd_mutext. 02/15 to 07/15 of this set cleans up
>>> code in md.c, and they
>>> are not very important for the rest of the set (is this correct?).
>>>
>>> Heming, you mentioned "the solution may simply return -EBUSY (instead
>>> of -ENODEV) to
>>> fail the open path". Could you please show the code? Maybe that would
>>> be enough to unblock
>>> the second half of Christoph's set (08/15 to 15/15)?
>>>
>>
>> I already sent the whole picture of md_open (email data: 2021-4-1,
>> subject: Re: [PATCH] md: don't create mddev in md_open).
>> My mail may fail to be delivered (at least, I got the "can't be delivered"
>> info on my last mail for linux-raid & guoqing's address). I put the related
>> email on attachment, anyone can read it again.
>>
>> For the convenience of discussion, I also pasted **patched** md_open below.
>> (I added more comments than enclosed email)
>>
>> ```
>> static int md_open(struct block_device *bdev, fmode_t mode)
>> {
>>      /* section 1 */
>>      struct mddev *mddev = mddev_find(bdev->bd_dev); //hm: the new style, only do searching job
>>      int err;
>>
>>      if (!mddev) //hm: this will cover freeing path 2.2 (refer enclosed file)
>>          return -ENODEV;
>>
>>      if (mddev->gendisk != bdev->bd_disk) { //hm: for freeing path 2.1 (refer enclosed file)
>>          /* we are racing with mddev_put which is discarding this
>>           * bd_disk.
>>           */
>>          mddev_put(mddev);
>>          /* Wait until bdev->bd_disk is definitely gone */
>>          if (work_pending(&mddev->del_work))
>>              flush_workqueue(md_misc_wq);
>>          return -EBUSY; //hm: fail this path. it also makes __blkdev_get return fail, userspace can try later.
>>                         //
>>                         // the legacy code flow:
>>                         //   return -ERESTARTSYS here, later __blkdev_get reentry.
>>                         //   it will trigger first 'if' in this functioin, then
>>                         //   return -ENODEV.
>>      }
>>
>>      /* section 2 */
>>      /* hm: below same as Christoph's [PATCH 01/15] */
>>      err = mutex_lock_interruptible(&mddev->open_mutex);
>>      if (err)
>>          return err;
>>
>>      if (test_bit(MD_CLOSING, &mddev->flags)) {
>>          mutex_unlock(&mddev->open_mutex);
>>          return -ENODEV;
>>      }
>>
>>      mddev_get(mddev);
>>      atomic_inc(&mddev->openers);
>>      mutex_unlock(&mddev->open_mutex);
>>
>>      bdev_check_media_change(bdev);
>>      return 0;
>> }
>> ```
>>
>> I wrote again:
>>> Christoph's patch [01/15] totally dropped <section 1>, and use bdev->bd_disk->private_data
>>> to get mddev pointer, it's not safe.
>>
>> And with above **patched** md_open, Christoph's patches 02-07 can be work happily.
>> He only needs to adjust/modify 01 patch.
>> The md layer behavior will change from return -ENODEV to -EBUSY in some racing scenario.
> 
> Thanks for the explanation.
> 
> Could you please send official patch of modified 01/15 and your fix
> (either in a set or merge into
> one patch)? This patch(set) would go to stable. Then, we can apply
> Christoph's 02 - 15 on top of
> it.
> 
> Thanks,
> Song
> 

Ok, I will resend a patch, which will replace my patch & Chistoph's patch 01.
Then Christoph could re-send 02-07 as v2 patch.
My patch need to work with Christoph's [PATCH 04/15] (md: split mddev_find) to fix soft lockup.
I will mention the relationship in my patch.

Thanks,
Heming