Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565312 > unrolled thread

Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> - 2017-01-23 22:20 +0100
    Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime Dan Williams <dan.j.williams@intel.com> - 2017-01-25 22:50 +0100
      Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime Jan Kara <jack@suse.cz> - 2017-01-26 11:10 +0100
        Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime Christoph Hellwig <hch@lst.de> - 2017-01-26 14:20 +0100
          Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime Dan Williams <dan.j.williams@intel.com> - 2017-01-26 17:50 +0100

#1565312 — Re: [RFC PATCH v2 0/2] block: fix backing_dev_info lifetime

Hello Dan,

Am Freitag, 6. Januar 2017, 17:02:51 BRST schrieb Dan Williams:
> v1 of these changes [1] was a one line change to bdev_get_queue() to
> prevent a shutdown crash when del_gendisk() races the final
> __blkdev_put().
> 
> While it is known at del_gendisk() time that the queue is still alive,
> Jan Kara points to other paths [2] that are racing __blkdev_put() where
> the assumption that ->bd_queue, or inode->i_wb is valid does not hold.
> 
> Fix that broken assumption, make it the case that if you have a live
> block_device, or block_device-inode that the corresponding queue and
> inode-write-back data is still valid.
> 
> These changes survive a run of the libnvdimm unit test suite which puts
> some stress on the block_device shutdown path.

I realize that the kernel test robot found problems with this series, but FWIW 
it fixes the bug mentioned in [2].

> [1]: http://marc.info/?l=linux-block&m=148366637105761&w=2
> [2]: http://www.spinics.net/lists/linux-fsdevel/msg105153.html

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[toc] | [next] | [standalone]

#1566984

On Mon, Jan 23, 2017 at 1:17 PM, Thiago Jung Bauermann
<bauerman@linux.vnet.ibm.com> wrote:
> Hello Dan,
>
> Am Freitag, 6. Januar 2017, 17:02:51 BRST schrieb Dan Williams:
>> v1 of these changes [1] was a one line change to bdev_get_queue() to
>> prevent a shutdown crash when del_gendisk() races the final
>> __blkdev_put().
>>
>> While it is known at del_gendisk() time that the queue is still alive,
>> Jan Kara points to other paths [2] that are racing __blkdev_put() where
>> the assumption that ->bd_queue, or inode->i_wb is valid does not hold.
>>
>> Fix that broken assumption, make it the case that if you have a live
>> block_device, or block_device-inode that the corresponding queue and
>> inode-write-back data is still valid.
>>
>> These changes survive a run of the libnvdimm unit test suite which puts
>> some stress on the block_device shutdown path.
>
> I realize that the kernel test robot found problems with this series, but FWIW
> it fixes the bug mentioned in [2].
>

Thanks for the test result. I might take a look at cleaning up the
test robot reports and resubmitting this approach unless Jan beats me
to the punch with his backing_devi_info lifetime change patches.

>> [2]: http://www.spinics.net/lists/linux-fsdevel/msg105153.html

[toc] | [prev] | [next] | [standalone]

#1567265

On Wed 25-01-17 13:43:58, Dan Williams wrote:
> On Mon, Jan 23, 2017 at 1:17 PM, Thiago Jung Bauermann
> <bauerman@linux.vnet.ibm.com> wrote:
> > Hello Dan,
> >
> > Am Freitag, 6. Januar 2017, 17:02:51 BRST schrieb Dan Williams:
> >> v1 of these changes [1] was a one line change to bdev_get_queue() to
> >> prevent a shutdown crash when del_gendisk() races the final
> >> __blkdev_put().
> >>
> >> While it is known at del_gendisk() time that the queue is still alive,
> >> Jan Kara points to other paths [2] that are racing __blkdev_put() where
> >> the assumption that ->bd_queue, or inode->i_wb is valid does not hold.
> >>
> >> Fix that broken assumption, make it the case that if you have a live
> >> block_device, or block_device-inode that the corresponding queue and
> >> inode-write-back data is still valid.
> >>
> >> These changes survive a run of the libnvdimm unit test suite which puts
> >> some stress on the block_device shutdown path.
> >
> > I realize that the kernel test robot found problems with this series, but FWIW
> > it fixes the bug mentioned in [2].
> >
> 
> Thanks for the test result. I might take a look at cleaning up the
> test robot reports and resubmitting this approach unless Jan beats me
> to the punch with his backing_devi_info lifetime change patches.

Yeah, so my patches (and I suspect your as well), have a problem when the
backing_device_info stays around because blkdev inode still exists, device
gets removed (e.g. USB disk gets unplugged) but blkdev inode still stays
around (there doesn't appear to be anything that would be forcing blkdev
inode out of cache on device removal and there cannot be because different
processes may hold inode reference) and then some other device gets plugged
in and reuses the same MAJOR:MINOR combination. Things get awkward there, I
think we need to unhash blkdev inode on device removal but so far I didn't
make this work...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1567420

On Thu, Jan 26, 2017 at 11:06:53AM +0100, Jan Kara wrote:
> Yeah, so my patches (and I suspect your as well), have a problem when the
> backing_device_info stays around because blkdev inode still exists, device
> gets removed (e.g. USB disk gets unplugged) but blkdev inode still stays
> around (there doesn't appear to be anything that would be forcing blkdev
> inode out of cache on device removal and there cannot be because different
> processes may hold inode reference) and then some other device gets plugged
> in and reuses the same MAJOR:MINOR combination. Things get awkward there, I
> think we need to unhash blkdev inode on device removal but so far I didn't
> make this work...

The other option is to simply not release the dev_t until the backing_dev
is gone.

[toc] | [prev] | [next] | [standalone]

#1567592

On Thu, Jan 26, 2017 at 5:17 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Jan 26, 2017 at 11:06:53AM +0100, Jan Kara wrote:
>> Yeah, so my patches (and I suspect your as well), have a problem when the
>> backing_device_info stays around because blkdev inode still exists, device
>> gets removed (e.g. USB disk gets unplugged) but blkdev inode still stays
>> around (there doesn't appear to be anything that would be forcing blkdev
>> inode out of cache on device removal and there cannot be because different
>> processes may hold inode reference) and then some other device gets plugged
>> in and reuses the same MAJOR:MINOR combination. Things get awkward there, I
>> think we need to unhash blkdev inode on device removal but so far I didn't
>> make this work...
>
> The other option is to simply not release the dev_t until the backing_dev
> is gone.

I came to a similar conclusion here:

   https://marc.info/?l=linux-scsi&m=147103737421897&w=4

James had some concerns, but I think its now clear this problem is
bigger than something we can fix locally in scsi.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web