From: Ming Lei <ming.lei@redhat.com>
To: Gulam Mohamed <gulam.mohamed@oracle.com>
Cc: viro@zeniv.linux.org.uk, axboe@kernel.dk,
linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, hch@lst.de,
martin.petersen@oracle.com, junxiao.bi@oracle.com
Subject: Re: [PATCH V1 1/1] Fix race between iscsi logout and systemd-udevd
Date: Wed, 12 May 2021 11:23:59 +0800 [thread overview]
Message-ID: <YJtKT7rLi2CFqDsV@T590> (raw)
In-Reply-To: <20210511181558.380764-1-gulam.mohamed@oracle.com>
On Tue, May 11, 2021 at 06:15:58PM +0000, Gulam Mohamed wrote:
> Problem description:
>
> During the kernel patching, customer was switching between the iscsi
> disks. To switch between the iscsi disks, it was logging out the
> currently connected iscsi disk and then logging in to the new iscsi
> disk. This was being done using a script. Customer was also using the
> "parted" command in the script to list the partition details just
> before the iscsi logout. This usage of "parted" command was creating
> an issue and we were seeing stale links of the
> disks in /sys/class/block.
>
> Analysis:
>
> As part of iscsi logout, the partitions and the disk will be removed
> in the function del_gendisk() which is done through a kworker. The
> parted command, used to list the partitions, will open the disk in
> RW mode which results in systemd-udevd re-reading the partitions. The
> ioctl used to re-read partitions is BLKRRPART. This will trigger the
> rescanning of partitions which will also delete and re-add the
> partitions. So, both iscsi logout processing (through kworker) and the
> "parted" command (through systemd-udevd) will be involved in
> add/delete of partitions. In our case, the following sequence of
> operations happened (the iscsi device is /dev/sdb with partition sdb1):
>
> 1. sdb1 was removed by PARTED
> 2. kworker, as part of iscsi logout, couldn't remove sdb1 as it was
> already removed by PARTED
> 3. sdb1 was added by parted
> 4. sdb was NOW removed as part of iscsi logout (the last part of the
> device removal after remoing the partitions)
>
> Since the symlink /sys/class/block/sdb1 points to
> /sys/class/devices/platform/hostx/sessionx/targetx:x/block/sdb/sdb1
> and since sdb is already removed, the symlink /sys/class/block/sdb1
> will be orphan and stale. So, this stale link is a result of the race
> condition in kernel between the systemd-udevd and iscsi-logout
> processing as described above. We were able to reproduce this even
> with latest upstream kernel.
>
> Fix:
>
> While Dropping/Adding partitions as part of BLKRRPART ioctl, take the
> read lock for "bdev_lookup_sem" to sync with del_gendisk().
>
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> ---
> fs/block_dev.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 09d6f7229db9..e903a7edfd63 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1245,9 +1245,17 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
> lockdep_assert_held(&bdev->bd_mutex);
>
> rescan:
> + down_read(&bdev_lookup_sem);
> + if (!(disk->flags & GENHD_FL_UP)) {
> + up_read(&bdev_lookup_sem);
> + return -ENXIO;
> + }
This way might cause deadlock:
1) code path BLKRRPART:
mutex_lock(bdev->bd_mutex)
down_read(&bdev_lookup_sem);
2) del_gendisk():
down_write(&bdev_lookup_sem);
mutex_lock(&disk->part0->bd_mutex);
Given GENHD_FL_UP is only checked when opening one bdev, and
fsync_bdev() and __invalidate_device() needn't to open bdev, so
the following way may work for your issue:
diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..5eb27995d4ab 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -617,6 +617,7 @@ void del_gendisk(struct gendisk *disk)
mutex_lock(&disk->part0->bd_mutex);
blk_drop_partitions(disk);
+ disk->flags &= ~GENHD_FL_UP;
mutex_unlock(&disk->part0->bd_mutex);
fsync_bdev(disk->part0);
@@ -629,7 +630,6 @@ void del_gendisk(struct gendisk *disk)
remove_inode_hash(disk->part0->bd_inode);
set_capacity(disk, 0);
- disk->flags &= ~GENHD_FL_UP;
up_write(&bdev_lookup_sem);
if (!(disk->flags & GENHD_FL_HIDDEN)) {
diff --git a/fs/block_dev.c b/fs/block_dev.c
index b8abccd03e5d..06b70b8e3f67 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1245,6 +1245,8 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
lockdep_assert_held(&bdev->bd_mutex);
rescan:
+ if(!(disk->flags & GENHD_FL_UP))
+ return -ENXIO;
if (bdev->bd_part_count)
return -EBUSY;
sync_blockdev(bdev);
Thanks,
Ming
next prev parent reply other threads:[~2021-05-12 3:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-11 18:15 [PATCH V1 1/1] Fix race between iscsi logout and systemd-udevd Gulam Mohamed
2021-05-12 3:23 ` Ming Lei [this message]
2021-05-12 6:35 ` Christoph Hellwig
2021-05-12 7:23 ` Ming Lei
2021-05-12 7:29 ` Christoph Hellwig
-- strict thread matches above, loose matches on Subject: below --
2021-04-27 10:13 Gulam Mohamed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YJtKT7rLi2CFqDsV@T590 \
--to=ming.lei@redhat.com \
--cc=axboe@kernel.dk \
--cc=gulam.mohamed@oracle.com \
--cc=hch@lst.de \
--cc=junxiao.bi@oracle.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).