| Message ID | 20240123-zonefs_nofs-v1-1-cc0b0308ef25@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: remove gfp_mask for blkdev_zone_mgmt() | expand |
On Tue, 23 Jan 2024, Johannes Thumshirn wrote: > Pass GFP_KERNEL instead of GFP_NOFS to the blkdev_zone_mgmt() call in > zonefs_zone_mgmt(). > > As as zonefs_zone_mgmt() and zonefs_inode_zone_mgmt() are never called > from a place that can recurse back into the filesystem on memory reclaim, > it is save to call blkdev_zone_mgmt() with GFP_KERNEL. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/zonefs/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c > index 93971742613a..63fbac018c04 100644 > --- a/fs/zonefs/super.c > +++ b/fs/zonefs/super.c > @@ -113,7 +113,7 @@ static int zonefs_zone_mgmt(struct super_block *sb, > > trace_zonefs_zone_mgmt(sb, z, op); > ret = blkdev_zone_mgmt(sb->s_bdev, op, z->z_sector, > - z->z_size >> SECTOR_SHIFT, GFP_NOFS); > + z->z_size >> SECTOR_SHIFT, GFP_KERNEL); > if (ret) { > zonefs_err(sb, > "Zone management operation %s at %llu failed %d\n", > > -- > 2.43.0 zonefs_inode_zone_mgmt calls lockdep_assert_held(&ZONEFS_I(inode)->i_truncate_mutex); - so, this function is called with the mutex held - could it happen that the GFP_KERNEL allocation recurses into the filesystem and attempts to take i_truncate_mutex as well? i.e. GFP_KERNEL -> iomap_do_writepage -> zonefs_write_map_blocks -> zonefs_write_iomap_begin -> mutex_lock(&zi->i_truncate_mutex) Mikulas
On Tue, Jan 23, 2024 at 09:39:02PM +0100, Mikulas Patocka wrote: > > > On Tue, 23 Jan 2024, Johannes Thumshirn wrote: > > > Pass GFP_KERNEL instead of GFP_NOFS to the blkdev_zone_mgmt() call in > > zonefs_zone_mgmt(). > > > > As as zonefs_zone_mgmt() and zonefs_inode_zone_mgmt() are never called > > from a place that can recurse back into the filesystem on memory reclaim, > > it is save to call blkdev_zone_mgmt() with GFP_KERNEL. > > > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > --- > > fs/zonefs/super.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c > > index 93971742613a..63fbac018c04 100644 > > --- a/fs/zonefs/super.c > > +++ b/fs/zonefs/super.c > > @@ -113,7 +113,7 @@ static int zonefs_zone_mgmt(struct super_block *sb, > > > > trace_zonefs_zone_mgmt(sb, z, op); > > ret = blkdev_zone_mgmt(sb->s_bdev, op, z->z_sector, > > - z->z_size >> SECTOR_SHIFT, GFP_NOFS); > > + z->z_size >> SECTOR_SHIFT, GFP_KERNEL); > > if (ret) { > > zonefs_err(sb, > > "Zone management operation %s at %llu failed %d\n", > > > > -- > > 2.43.0 > > zonefs_inode_zone_mgmt calls > lockdep_assert_held(&ZONEFS_I(inode)->i_truncate_mutex); - so, this > function is called with the mutex held - could it happen that the > GFP_KERNEL allocation recurses into the filesystem and attempts to take > i_truncate_mutex as well? > > i.e. GFP_KERNEL -> iomap_do_writepage -> zonefs_write_map_blocks -> > zonefs_write_iomap_begin -> mutex_lock(&zi->i_truncate_mutex) zonefs doesn't have a ->writepage method, so writeback can't be called from memory reclaim like this. -Dave.
On 1/24/24 08:21, Dave Chinner wrote: > On Tue, Jan 23, 2024 at 09:39:02PM +0100, Mikulas Patocka wrote: >> >> >> On Tue, 23 Jan 2024, Johannes Thumshirn wrote: >> >>> Pass GFP_KERNEL instead of GFP_NOFS to the blkdev_zone_mgmt() call in >>> zonefs_zone_mgmt(). >>> >>> As as zonefs_zone_mgmt() and zonefs_inode_zone_mgmt() are never called >>> from a place that can recurse back into the filesystem on memory reclaim, >>> it is save to call blkdev_zone_mgmt() with GFP_KERNEL. >>> >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> --- >>> fs/zonefs/super.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c >>> index 93971742613a..63fbac018c04 100644 >>> --- a/fs/zonefs/super.c >>> +++ b/fs/zonefs/super.c >>> @@ -113,7 +113,7 @@ static int zonefs_zone_mgmt(struct super_block *sb, >>> >>> trace_zonefs_zone_mgmt(sb, z, op); >>> ret = blkdev_zone_mgmt(sb->s_bdev, op, z->z_sector, >>> - z->z_size >> SECTOR_SHIFT, GFP_NOFS); >>> + z->z_size >> SECTOR_SHIFT, GFP_KERNEL); >>> if (ret) { >>> zonefs_err(sb, >>> "Zone management operation %s at %llu failed %d\n", >>> >>> -- >>> 2.43.0 >> >> zonefs_inode_zone_mgmt calls >> lockdep_assert_held(&ZONEFS_I(inode)->i_truncate_mutex); - so, this >> function is called with the mutex held - could it happen that the >> GFP_KERNEL allocation recurses into the filesystem and attempts to take >> i_truncate_mutex as well? >> >> i.e. GFP_KERNEL -> iomap_do_writepage -> zonefs_write_map_blocks -> >> zonefs_write_iomap_begin -> mutex_lock(&zi->i_truncate_mutex) > > zonefs doesn't have a ->writepage method, so writeback can't be > called from memory reclaim like this. And also, buffered writes are allowed only for conventional zone files, for which we never do zone management. For sequential zone files which may have there zone managed with blkdev_zone_mgmt(), only direct writes are allowed. > > -Dave.
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c index 93971742613a..63fbac018c04 100644 --- a/fs/zonefs/super.c +++ b/fs/zonefs/super.c @@ -113,7 +113,7 @@ static int zonefs_zone_mgmt(struct super_block *sb, trace_zonefs_zone_mgmt(sb, z, op); ret = blkdev_zone_mgmt(sb->s_bdev, op, z->z_sector, - z->z_size >> SECTOR_SHIFT, GFP_NOFS); + z->z_size >> SECTOR_SHIFT, GFP_KERNEL); if (ret) { zonefs_err(sb, "Zone management operation %s at %llu failed %d\n",
Pass GFP_KERNEL instead of GFP_NOFS to the blkdev_zone_mgmt() call in zonefs_zone_mgmt(). As as zonefs_zone_mgmt() and zonefs_inode_zone_mgmt() are never called from a place that can recurse back into the filesystem on memory reclaim, it is save to call blkdev_zone_mgmt() with GFP_KERNEL. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/zonefs/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)