Hi,
在 2023/01/30 21:17, James Bottomley 写道:
On Mon, 2023-01-30 at 11:46 +0800, Yu Kuai wrote:
Hi,
在 2023/01/30 11:29, James Bottomley 写道:
On Mon, 2023-01-30 at 11:07 +0800, Yu Kuai wrote:
Hi,
在 2023/01/30 1:30, James Bottomley 写道:
On Sat, 2023-01-28 at 17:41 +0800, Zhong Jinghua wrote:
This error will cause a warning:
kobject_add_internal failed for block (error: -2 parent:
1:0:0:1). In the lower version (such as 5.10), there is no
corresponding error handling, continuing to go down will
trigger a kernel panic, so cc stable.
Is this is important point and what you're saying is that this
only panics on kernels before 5.10 or so because after that
it's correctly failed by block device error handling so there's
nothing to fix in later kernels?
In that case, isn't the correct fix to look at backporting the
block device error handling:
This is the last commit that support error handling, and there
are many relied patches, and there are lots of refactor in block
layer. It's not a good idea to backport error handling to lower
version.
Althrough error handling can prevent kernel crash in this case, I
still think it make sense to make sure kobject is deleted in
order, parent should not be deleted before child.
Well, look, you've created a very artificial situation where a
create closely followed by a delete of the underlying sdev races
with the create of the block gendisk devices of sd that bind
asynchronously to the created sdev. The asynchronous nature of the
bind gives the elongated race window so the only real fix is some
sort of check that the sdev is still viable by the time the bind
occurs ... probably in sd_probe(), say a scsi_device_get of sdp at
the top which would ensure viability of the sdev for the entire
bind or fail the probe if the sdev can't be got.
Sorry, I don't follow here. 😟
In the current kernel the race is mitigated because add_device fails
due to the parent being torn down. That parent is the sdev->gendev so
it seems we can detect this in the probe by looking at the sdev->gendev
state, which scsi_device_get() will do.
I agree this is a very artificial situation, however I can't tell our
tester not to test this way...
The problem is that kobject session is deleted and then sd_probe()
tries to create a new kobject under hostx/sessionx/x:x:x:x/. I don't
see how scsi_device_get() can prevent that, it only get a kobject
reference and can prevent kobject to be released, however,
kobject_del() can still be done.
So your contention is there's no way that we could make scsi_device_get
see the kernfs deactivation? I would have thought checking sdev-
sdev_gendev.kobj.sd.active would give that ... although the check
would have to be via an API since KN_DEACTIVATED_BIAS is internal.
I'm still not sure if such checking is enough.
session1/target1:0:0/1:0:0:0/block
1) t1 is deleting target, and t1 already set 1:0:0:0 to SDEV_CANCEL, and
1:0:0:0 is not deleted yet.
2) t2 is deleting session1, 1:0:0:0 state is SDEV_CACEL, so 1:0:0:0 is
skipped, and session1 is deleted before 1:0:0:0, which will cause
1:0:0:0 to be not active.
3) t3 create block, it can happen because 1:0:0:0 is still not deleted,
and later kobject_add() will found 1:0:0:0 is not active and hence
faild.
The problem is that deleting parent kobject will cause child kobject not
to be active, and in 3) device_lock is not hold for parents, hence just
checking if this scsi_device is active is not enough, we have to make
sure parents won't be deleted concurrently, for example, a litter
adjustment for above procedures:
1) ...(the same)
2) t3 create block, it check kobject state is still active
3) t2 delete session1 ...(the same), 1:0:0:0 is not active anymore.
4) t3 continue to create block undre 1:0:0:0, which will fail.
By the way, I think such problem exist because scsi_device state is
SDEV_CANCEL doesn't mean that the device is deleted, simply skip such
device while removing session is not right.
Do you found other problems if we make sure that kobject is deleted in
order?
Thanks,
Kuai
James
In this patch, we make sure remove session and sd_probe() won't
concurrent, remove session will wait for all child kobject to be
deleted, what do you think?
Thanks,
Kuai
.
