Skip to content

Commit 5e3b723

Browse files
author
Kent Overstreet
committed
bcachefs: Fix sysfs warning in fstests generic/730,731
sysfs warns if we're removing a symlink from a directory that's no longer in sysfs; this is triggered by fstests generic/730, which simulates hot removal of a block device. This patch is however not a correct fix, since checking kobj->state_in_sysfs on a kobj owned by another subsystem is racy. A better fix would be to add the appropriate check to sysfs_remove_link() - and sysfs_create_link() as well. But kobject_add_internal()/kobject_del() do not as of today have locking that would support that. Note that the block/holder.c code appears to be subject to this race as well. Cc: Greg Kroah-Hartman <[email protected]> Cc: "Rafael J. Wysocki" <[email protected]> Cc: Christoph Hellwig <[email protected]> Signed-off-by: Kent Overstreet <[email protected]>
1 parent cb6055e commit 5e3b723

File tree

1 file changed

+24
-10
lines changed

1 file changed

+24
-10
lines changed

fs/bcachefs/super.c

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ static DEFINE_MUTEX(bch_fs_list_lock);
184184

185185
DECLARE_WAIT_QUEUE_HEAD(bch2_read_only_wait);
186186

187+
static void bch2_dev_unlink(struct bch_dev *);
187188
static void bch2_dev_free(struct bch_dev *);
188189
static int bch2_dev_alloc(struct bch_fs *, unsigned);
189190
static int bch2_dev_sysfs_online(struct bch_fs *, struct bch_dev *);
@@ -620,9 +621,7 @@ void __bch2_fs_stop(struct bch_fs *c)
620621
up_write(&c->state_lock);
621622

622623
for_each_member_device(c, ca)
623-
if (ca->kobj.state_in_sysfs &&
624-
ca->disk_sb.bdev)
625-
sysfs_remove_link(bdev_kobj(ca->disk_sb.bdev), "bcachefs");
624+
bch2_dev_unlink(ca);
626625

627626
if (c->kobj.state_in_sysfs)
628627
kobject_del(&c->kobj);
@@ -1187,9 +1186,7 @@ static void bch2_dev_free(struct bch_dev *ca)
11871186
{
11881187
cancel_work_sync(&ca->io_error_work);
11891188

1190-
if (ca->kobj.state_in_sysfs &&
1191-
ca->disk_sb.bdev)
1192-
sysfs_remove_link(bdev_kobj(ca->disk_sb.bdev), "bcachefs");
1189+
bch2_dev_unlink(ca);
11931190

11941191
if (ca->kobj.state_in_sysfs)
11951192
kobject_del(&ca->kobj);
@@ -1226,10 +1223,7 @@ static void __bch2_dev_offline(struct bch_fs *c, struct bch_dev *ca)
12261223
percpu_ref_kill(&ca->io_ref);
12271224
wait_for_completion(&ca->io_ref_completion);
12281225

1229-
if (ca->kobj.state_in_sysfs) {
1230-
sysfs_remove_link(bdev_kobj(ca->disk_sb.bdev), "bcachefs");
1231-
sysfs_remove_link(&ca->kobj, "block");
1232-
}
1226+
bch2_dev_unlink(ca);
12331227

12341228
bch2_free_super(&ca->disk_sb);
12351229
bch2_dev_journal_exit(ca);
@@ -1251,6 +1245,26 @@ static void bch2_dev_io_ref_complete(struct percpu_ref *ref)
12511245
complete(&ca->io_ref_completion);
12521246
}
12531247

1248+
static void bch2_dev_unlink(struct bch_dev *ca)
1249+
{
1250+
struct kobject *b;
1251+
1252+
/*
1253+
* This is racy w.r.t. the underlying block device being hot-removed,
1254+
* which removes it from sysfs.
1255+
*
1256+
* It'd be lovely if we had a way to handle this race, but the sysfs
1257+
* code doesn't appear to provide a good method and block/holder.c is
1258+
* susceptible as well:
1259+
*/
1260+
if (ca->kobj.state_in_sysfs &&
1261+
ca->disk_sb.bdev &&
1262+
(b = bdev_kobj(ca->disk_sb.bdev))->state_in_sysfs) {
1263+
sysfs_remove_link(b, "bcachefs");
1264+
sysfs_remove_link(&ca->kobj, "block");
1265+
}
1266+
}
1267+
12541268
static int bch2_dev_sysfs_online(struct bch_fs *c, struct bch_dev *ca)
12551269
{
12561270
int ret;

0 commit comments

Comments
 (0)