| Message ID | 20231101174325.10596-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | block: Add config option to not allow writing to mounted devices | expand |
On Wed, Nov 01, 2023 at 06:43:06PM +0100, Jan Kara wrote: > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > CC: Kent Overstreet <kent.overstreet@linux.dev> > CC: Brian Foster <bfoster@redhat.com> > CC: linux-bcachefs@vger.kernel.org > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/bcachefs/super-io.c | 19 ++++++++++--------- > fs/bcachefs/super_types.h | 1 + > 2 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/fs/bcachefs/super-io.c b/fs/bcachefs/super-io.c > index 332d41e1c0a3..01a32c41a540 100644 > --- a/fs/bcachefs/super-io.c > +++ b/fs/bcachefs/super-io.c ... > @@ -685,21 +685,22 @@ int bch2_read_super(const char *path, struct bch_opts *opts, > if (!opt_get(*opts, nochanges)) > sb->mode |= BLK_OPEN_WRITE; > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > - if (IS_ERR(sb->bdev) && > - PTR_ERR(sb->bdev) == -EACCES && > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > + if (IS_ERR(sb->bdev_handle) && > + PTR_ERR(sb->bdev_handle) == -EACCES && > opt_get(*opts, read_only)) { > sb->mode &= ~BLK_OPEN_WRITE; > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > - if (!IS_ERR(sb->bdev)) > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > + if (!IS_ERR(sb->bdev_handle)) > opt_set(*opts, nochanges, true); > } > > - if (IS_ERR(sb->bdev)) { > - ret = PTR_ERR(sb->bdev); > + if (IS_ERR(sb->bdev_handle)) { > + ret = PTR_ERR(sb->bdev_handle); > goto out; > } > + sb->bdev = sb->bdev_handle->bdev; > > ret = bch2_sb_realloc(sb, 0); > if (ret) { Hi Jan, This all seems reasonable to me, but should bcachefs use sb_open_mode() somewhere in here to involve use of the restrict writes flag in the first place? It looks like bcachefs sort of open codes bits of mount_bdev() so it isn't clear the flag would be used anywhere... Brian > diff --git a/fs/bcachefs/super_types.h b/fs/bcachefs/super_types.h > index 78d6138db62d..b77d8897c9fa 100644 > --- a/fs/bcachefs/super_types.h > +++ b/fs/bcachefs/super_types.h > @@ -4,6 +4,7 @@ > > struct bch_sb_handle { > struct bch_sb *sb; > + struct bdev_handle *bdev_handle; > struct block_device *bdev; > struct bio *bio; > void *holder; > -- > 2.35.3 >
On Wed, Nov 01, 2023 at 03:01:22PM -0400, Brian Foster wrote: > On Wed, Nov 01, 2023 at 06:43:06PM +0100, Jan Kara wrote: > > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > > > CC: Kent Overstreet <kent.overstreet@linux.dev> > > CC: Brian Foster <bfoster@redhat.com> > > CC: linux-bcachefs@vger.kernel.org > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/bcachefs/super-io.c | 19 ++++++++++--------- > > fs/bcachefs/super_types.h | 1 + > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/fs/bcachefs/super-io.c b/fs/bcachefs/super-io.c > > index 332d41e1c0a3..01a32c41a540 100644 > > --- a/fs/bcachefs/super-io.c > > +++ b/fs/bcachefs/super-io.c > ... > > @@ -685,21 +685,22 @@ int bch2_read_super(const char *path, struct bch_opts *opts, > > if (!opt_get(*opts, nochanges)) > > sb->mode |= BLK_OPEN_WRITE; > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > - if (IS_ERR(sb->bdev) && > > - PTR_ERR(sb->bdev) == -EACCES && > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > + if (IS_ERR(sb->bdev_handle) && > > + PTR_ERR(sb->bdev_handle) == -EACCES && > > opt_get(*opts, read_only)) { > > sb->mode &= ~BLK_OPEN_WRITE; > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > - if (!IS_ERR(sb->bdev)) > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > + if (!IS_ERR(sb->bdev_handle)) > > opt_set(*opts, nochanges, true); > > } > > > > - if (IS_ERR(sb->bdev)) { > > - ret = PTR_ERR(sb->bdev); > > + if (IS_ERR(sb->bdev_handle)) { > > + ret = PTR_ERR(sb->bdev_handle); > > goto out; > > } > > + sb->bdev = sb->bdev_handle->bdev; > > > > ret = bch2_sb_realloc(sb, 0); > > if (ret) { > > Hi Jan, > > This all seems reasonable to me, but should bcachefs use sb_open_mode() > somewhere in here to involve use of the restrict writes flag in the > first place? It looks like bcachefs sort of open codes bits of > mount_bdev() so it isn't clear the flag would be used anywhere... Yeah, but that should be a separate patch :)
On Wed, Nov 01, 2023 at 06:43:06PM +0100, Jan Kara wrote: > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > CC: Kent Overstreet <kent.overstreet@linux.dev> > CC: Brian Foster <bfoster@redhat.com> > CC: linux-bcachefs@vger.kernel.org > Signed-off-by: Jan Kara <jack@suse.cz> Acked-by: Kent Overstreet <kent.overstreet@linux.dev>
Hi Brian, On Wed 01-11-23 15:01:22, Brian Foster wrote: > On Wed, Nov 01, 2023 at 06:43:06PM +0100, Jan Kara wrote: > > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > > > CC: Kent Overstreet <kent.overstreet@linux.dev> > > CC: Brian Foster <bfoster@redhat.com> > > CC: linux-bcachefs@vger.kernel.org > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/bcachefs/super-io.c | 19 ++++++++++--------- > > fs/bcachefs/super_types.h | 1 + > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/fs/bcachefs/super-io.c b/fs/bcachefs/super-io.c > > index 332d41e1c0a3..01a32c41a540 100644 > > --- a/fs/bcachefs/super-io.c > > +++ b/fs/bcachefs/super-io.c > ... > > @@ -685,21 +685,22 @@ int bch2_read_super(const char *path, struct bch_opts *opts, > > if (!opt_get(*opts, nochanges)) > > sb->mode |= BLK_OPEN_WRITE; > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > - if (IS_ERR(sb->bdev) && > > - PTR_ERR(sb->bdev) == -EACCES && > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > + if (IS_ERR(sb->bdev_handle) && > > + PTR_ERR(sb->bdev_handle) == -EACCES && > > opt_get(*opts, read_only)) { > > sb->mode &= ~BLK_OPEN_WRITE; > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > - if (!IS_ERR(sb->bdev)) > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > + if (!IS_ERR(sb->bdev_handle)) > > opt_set(*opts, nochanges, true); > > } > > > > - if (IS_ERR(sb->bdev)) { > > - ret = PTR_ERR(sb->bdev); > > + if (IS_ERR(sb->bdev_handle)) { > > + ret = PTR_ERR(sb->bdev_handle); > > goto out; > > } > > + sb->bdev = sb->bdev_handle->bdev; > > > > ret = bch2_sb_realloc(sb, 0); > > if (ret) { > > This all seems reasonable to me, but should bcachefs use sb_open_mode() > somewhere in here to involve use of the restrict writes flag in the > first place? It looks like bcachefs sort of open codes bits of > mount_bdev() so it isn't clear the flag would be used anywhere... Yes, so AFAICS bcachefs will not get the write restriction from the changes in the generic code. Using sb_open_mode() in bcachefs would fix that but given the 'noexcl' and 'nochanges' mount options it will not be completely seamless anyway. I guess once the generic changes are in, bcachefs can decide how exactly it wants to set the BLK_OPEN_RESTRICT_WRITES flag. Or if you already have an opinion, we can just add the patch to this series. Honza
On Thu, Nov 02, 2023 at 10:55:50AM +0100, Jan Kara wrote: > Hi Brian, > > On Wed 01-11-23 15:01:22, Brian Foster wrote: > > On Wed, Nov 01, 2023 at 06:43:06PM +0100, Jan Kara wrote: > > > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > > > > > CC: Kent Overstreet <kent.overstreet@linux.dev> > > > CC: Brian Foster <bfoster@redhat.com> > > > CC: linux-bcachefs@vger.kernel.org > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/bcachefs/super-io.c | 19 ++++++++++--------- > > > fs/bcachefs/super_types.h | 1 + > > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/bcachefs/super-io.c b/fs/bcachefs/super-io.c > > > index 332d41e1c0a3..01a32c41a540 100644 > > > --- a/fs/bcachefs/super-io.c > > > +++ b/fs/bcachefs/super-io.c > > ... > > > @@ -685,21 +685,22 @@ int bch2_read_super(const char *path, struct bch_opts *opts, > > > if (!opt_get(*opts, nochanges)) > > > sb->mode |= BLK_OPEN_WRITE; > > > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > > - if (IS_ERR(sb->bdev) && > > > - PTR_ERR(sb->bdev) == -EACCES && > > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > > + if (IS_ERR(sb->bdev_handle) && > > > + PTR_ERR(sb->bdev_handle) == -EACCES && > > > opt_get(*opts, read_only)) { > > > sb->mode &= ~BLK_OPEN_WRITE; > > > > > > - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > > - if (!IS_ERR(sb->bdev)) > > > + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); > > > + if (!IS_ERR(sb->bdev_handle)) > > > opt_set(*opts, nochanges, true); > > > } > > > > > > - if (IS_ERR(sb->bdev)) { > > > - ret = PTR_ERR(sb->bdev); > > > + if (IS_ERR(sb->bdev_handle)) { > > > + ret = PTR_ERR(sb->bdev_handle); > > > goto out; > > > } > > > + sb->bdev = sb->bdev_handle->bdev; > > > > > > ret = bch2_sb_realloc(sb, 0); > > > if (ret) { > > > > This all seems reasonable to me, but should bcachefs use sb_open_mode() > > somewhere in here to involve use of the restrict writes flag in the > > first place? It looks like bcachefs sort of open codes bits of > > mount_bdev() so it isn't clear the flag would be used anywhere... > > Yes, so AFAICS bcachefs will not get the write restriction from the changes > in the generic code. Using sb_open_mode() in bcachefs would fix that but > given the 'noexcl' and 'nochanges' mount options it will not be completely > seamless anyway. I guess once the generic changes are in, bcachefs can > decide how exactly it wants to set the BLK_OPEN_RESTRICT_WRITES flag. Or if > you already have an opinion, we can just add the patch to this series. > Yep, makes sense, and I agree with Kent this should be a separate patch anyways. Just wanted to make sure I wasn't missing something or otherwise it was known that bcachefs needs a bit more work to turn this on... thanks. Brian > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR >
On Wed, 1 Nov 2023 18:43:06 +0100, Jan Kara wrote: > Convert bcachefs to use bdev_open_by_path() and pass the handle around. > > Applied to the vfs.super branch of the vfs/vfs.git tree. Patches in the vfs.super branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.super [1/7] bcachefs: Convert to bdev_open_by_path() https://git.kernel.org/vfs/vfs/c/8e897399352c [2/7] block: Remove blkdev_get_by_*() functions https://git.kernel.org/vfs/vfs/c/1dc2789bf2d9 [3/7] block: Add config option to not allow writing to mounted devices https://git.kernel.org/vfs/vfs/c/708e8ecda49e [4/7] btrfs: Do not restrict writes to btrfs devices https://git.kernel.org/vfs/vfs/c/b6b2f4843264 [5/7] fs: Block writes to mounted block devices https://git.kernel.org/vfs/vfs/c/48ce483465bb [6/7] xfs: Block writes to log device https://git.kernel.org/vfs/vfs/c/dae1e956882c [7/7] ext4: Block writes to journal device https://git.kernel.org/vfs/vfs/c/a8a97da12393
diff --git a/fs/bcachefs/super-io.c b/fs/bcachefs/super-io.c index 332d41e1c0a3..01a32c41a540 100644 --- a/fs/bcachefs/super-io.c +++ b/fs/bcachefs/super-io.c @@ -162,8 +162,8 @@ void bch2_sb_field_delete(struct bch_sb_handle *sb, void bch2_free_super(struct bch_sb_handle *sb) { kfree(sb->bio); - if (!IS_ERR_OR_NULL(sb->bdev)) - blkdev_put(sb->bdev, sb->holder); + if (!IS_ERR_OR_NULL(sb->bdev_handle)) + bdev_release(sb->bdev_handle); kfree(sb->holder); kfree(sb->sb); @@ -685,21 +685,22 @@ int bch2_read_super(const char *path, struct bch_opts *opts, if (!opt_get(*opts, nochanges)) sb->mode |= BLK_OPEN_WRITE; - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); - if (IS_ERR(sb->bdev) && - PTR_ERR(sb->bdev) == -EACCES && + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); + if (IS_ERR(sb->bdev_handle) && + PTR_ERR(sb->bdev_handle) == -EACCES && opt_get(*opts, read_only)) { sb->mode &= ~BLK_OPEN_WRITE; - sb->bdev = blkdev_get_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); - if (!IS_ERR(sb->bdev)) + sb->bdev_handle = bdev_open_by_path(path, sb->mode, sb->holder, &bch2_sb_handle_bdev_ops); + if (!IS_ERR(sb->bdev_handle)) opt_set(*opts, nochanges, true); } - if (IS_ERR(sb->bdev)) { - ret = PTR_ERR(sb->bdev); + if (IS_ERR(sb->bdev_handle)) { + ret = PTR_ERR(sb->bdev_handle); goto out; } + sb->bdev = sb->bdev_handle->bdev; ret = bch2_sb_realloc(sb, 0); if (ret) { diff --git a/fs/bcachefs/super_types.h b/fs/bcachefs/super_types.h index 78d6138db62d..b77d8897c9fa 100644 --- a/fs/bcachefs/super_types.h +++ b/fs/bcachefs/super_types.h @@ -4,6 +4,7 @@ struct bch_sb_handle { struct bch_sb *sb; + struct bdev_handle *bdev_handle; struct block_device *bdev; struct bio *bio; void *holder;
Convert bcachefs to use bdev_open_by_path() and pass the handle around. CC: Kent Overstreet <kent.overstreet@linux.dev> CC: Brian Foster <bfoster@redhat.com> CC: linux-bcachefs@vger.kernel.org Signed-off-by: Jan Kara <jack@suse.cz> --- fs/bcachefs/super-io.c | 19 ++++++++++--------- fs/bcachefs/super_types.h | 1 + 2 files changed, 11 insertions(+), 9 deletions(-)