From: Jan Kara <jack@suse.cz>
Sent: Wednesday, June 4, 2025 7:53 PM

Hi!

On Tue 03-06-25 13:33:02, Parav Pandit wrote:

quoted

quoted

From: Parav Pandit <redacted>
Sent: Tuesday, May 27, 2025 7:55 PM

quoted

From: Jan Kara <jack@suse.cz>
Sent: Tuesday, May 27, 2025 7:51 PM

On Tue 27-05-25 12:07:20, Parav Pandit wrote:

quoted

quoted

From: Jan Kara <jack@suse.cz>
Sent: Tuesday, May 27, 2025 5:27 PM

On Tue 27-05-25 11:00:56, Parav Pandit wrote:

quoted

quoted

From: Jan Kara <jack@suse.cz>
Sent: Monday, May 26, 2025 10:09 PM

Hello!

On Sat 24-05-25 05:56:55, Parav Pandit wrote:

quoted

I am running a basic test of block device driver unbind,
bind while the fio is running random write IOs with
direct=0.  The test hits the WARN_ON assert on:

void pagecache_isize_extended(struct inode *inode,
loff_t from, loff_t
to) {
        int bsize = i_blocksize(inode);
        loff_t rounded_from;
        struct folio *folio;

        WARN_ON(to > inode->i_size);

This is because when the block device is removed during
driver unbind, the driver flow is,

del_gendisk()
    __blk_mark_disk_dead()
            set_capacity((disk, 0);
                bdev_set_nr_sectors()
                    i_size_write() -> This will set the
inode's isize to 0, while the

page cache is yet to be flushed.

quoted

Below is the kernel call trace.

Can someone help to identify, where should be the fix?
Should block layer to not set the capacity to 0?
Or page catch to overcome this dynamic changing of the size?
Or?

After thinking about this the proper fix would be for
i_size_write() to happen under i_rwsem because the change
in the middle of the write is what's confusing the iomap
code. I smell some deadlock potential here but it's
perhaps worth trying :)

Without it, I gave a quick try with inode_lock() unlock() in
i_size_write() and initramfs level it was stuck.  I am yet
to try with LOCKDEP.

You definitely cannot put inode_lock() into i_size_write().
i_size_write() is expected to be called under inode_lock. And
bdev_set_nr_sectors() is breaking this rule by not holding it.
So what you can try is to do
inode_lock() in bdev_set_nr_sectors() instead of grabbing

bd_size_lock.

quoted

quoted

quoted

quoted

quoted

I replaced the bd_size_lock with inode_lock().
Was unable to reproduce the issue yet with the fix.

However, it right away breaks the Atari floppy driver who invokes
set_capacity() in queue_rq() at [1]. !!

[1]
https://elixir.bootlin.com/linux/v6.15/source/drivers/block/ataflop.c#
L1544

Yeah, that's somewhat unexpected. Anyway, Atari floppy driver isn't exactly a
reliable source of proper locking...

True. I find it weird for one to change the capacity in middle of serving the block request.
Must be very old code.

quoted

With my limited knowledge I find the fix risky as bottom block layer
is invoking upper FS layer inode lock.  I suspect it may lead to A->B,
B->A locking in some path.

Well, I'm suspecting that as well. That's why I've asked you to test it :) Are you
running with lockdep enabled? Because I'd expect it to complain rather quickly.
Possibly run blktests as well to exercise various not-so-usual paths so that
lockdep learns about various lock dependencies.

Yes, with lockdep enabled. Nothing reported in this area.
But its test with only single block device = virtio block and with LSI Fusion MPT SAS scsi drives.

Seeing lock dep asserts in other areas not on block side yet. 😊

quoted

Other than Atari floppy driver, I didn't find any other offending
driver, but its hard to say, its safe from A->B, B->A deadlock.
A = inode lock
B = block driver level lock

								Honza

quoted

quoted

quoted

quoted

Ok. will try this.
I am off for few days on travel, so earliest I can do is on Sunday.

quoted

quoted

I was thinking, can the existing sequence lock be used for
64-bit case as well?

The sequence lock is about updating inode->i_size value itself.
But we need much larger scale protection here - we need to
make sure write to the block device is not happening while the
device size changes. And that's what inode_lock is usually used for.

Other option to explore (with my limited knowledge) is, When the
block device is removed, not to update the size,

Because queue dying flag and other barriers are placed to
prevent the IOs

entering lower layer or to fail them.

quoted

Can that be the direction to fix?

Well, that's definitely one line of defense and it's enough for
reads but for writes you don't want them to accumulate in the page
cache (and thus consume memory) when you know you have no way to
write

them

quoted

out. So there needs to be some way for buffered writes to
recognize the backing store is gone and stop them before dirtying pages.
Currently that's achieved by reducing i_size, we can think of
other mechanisms but reducing i_size is kind of elegant if we can
synchronize that

properly...

quoted

The block device notifies the bio layer by calling
blk_queue_flag_set(QUEUE_FLAG_DYING, disk->queue); Maybe we can

come

quoted

quoted

up with notification method that updates some flag to page cache
layer to drop buffered writes to floor.

Or other direction to explore, if the WAR_ON() is still valid, as it
can change anytime?

quoted

								Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR

--
Jan Kara [off-list ref]
SUSE Labs, CR