| Message ID | cover.1700673401.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: convert to the new mount API | expand |
On Wed, Nov 22, 2023 at 12:18 PM Josef Bacik <josef@toxicpanda.com> wrote: > > v2->v3: > - Fixed up the various review comments from Dave and Anand. > - Added a patch to drop the deprecated mount options we currently have. > > v1->v2: > - Fixed up some nits and paste errors. > - Fixed build failure with !ZONED. > - Fixed accidentally dropping BINARY_MOUNTDATA flag. > - Added Reviewed-by's collected up to this point. > > These have run through our CI a few times, they haven't introduced any > regressions. > > --- Original email --- > Hello, > > These patches convert us to use the new mount API. Christian tried to do this a > few months ago, but ran afoul of our preference to have a bunch of small > changes. I started this series before I knew he had tried to convert us, so > there's a fair bit that's different, but I did copy his approach for the remount > bit. I've linked to the original patch where I took inspiration, Christian let > me know if you want some other annotation for credit, I wasn't really sure the > best way to do that. > > There are a few preparatory patches in the beginning, and then cleanups at the > end. I took each call back one at a time to try and make it as small as > possible. The resulting code is less, but the diffstat shows more insertions > that deletions. This is because there are some big comment blocks around some > of the more subtle things that we're doing to hopefully make it more clear. > > This is currently running through our CI. I thought it was fine last week but > we had a bunch of new failures when I finished up the remount behavior. However > today I discovered this was a regression in btrfs-progs, and I'm re-running the > tests with the fixes. If anything major breaks in the CI I'll resend with > fixes, but I'm pretty sure these patches will pass without issue. > > I utilized __maybe_unused liberally to make sure everything compiled while > applied. The only "big" patch is where I went and removed the old API. If > requested I can break that up a bit more, but I didn't think it was necessary. > I did make sure to keep it in its own patch, so the switch to the new mount API > path only has things we need to support the new mount API, and then the next > patch removes the old code. Thanks, > > Josef > > Christian Brauner (1): > fs: indicate request originates from old mount api > > Josef Bacik (18): > btrfs: split out the mount option validation code into its own helper > btrfs: set default compress type at btrfs_init_fs_info time > btrfs: move space cache settings into open_ctree > btrfs: do not allow free space tree rebuild on extent tree v2 > btrfs: split out ro->rw and rw->ro helpers into their own functions > btrfs: add a NOSPACECACHE mount option flag > btrfs: add fs_parameter definitions > btrfs: add parse_param callback for the new mount api > btrfs: add fs context handling functions > btrfs: add reconfigure callback for fs_context > btrfs: add get_tree callback for new mount API > btrfs: handle the ro->rw transition for mounting different subovls > btrfs: switch to the new mount API > btrfs: move the device specific mount options to super.c > btrfs: remove old mount API code > btrfs: move one shot mount option clearing to super.c > btrfs: set clear_cache if we use usebackuproot > btrfs: remove code for inode_cache and recovery mount options > > fs/btrfs/disk-io.c | 85 +- > fs/btrfs/disk-io.h | 1 - > fs/btrfs/fs.h | 15 +- > fs/btrfs/super.c | 2357 +++++++++++++++++++++++--------------------- > fs/btrfs/super.h | 5 +- > fs/btrfs/zoned.c | 16 +- > fs/btrfs/zoned.h | 6 +- > fs/namespace.c | 11 + > 8 files changed, 1263 insertions(+), 1233 deletions(-) > > -- > 2.41.0 > Looks like my r-b wasn't picked up for this revision, but looking over it, things seem to be fine. Reviewed-by: Neal Gompa <neal@gompa.dev>
On Wed, Nov 22, 2023 at 12:41:30PM -0500, Neal Gompa wrote: > On Wed, Nov 22, 2023 at 12:18 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > v2->v3: > > - Fixed up the various review comments from Dave and Anand. > > - Added a patch to drop the deprecated mount options we currently have. > > > > v1->v2: > > - Fixed up some nits and paste errors. > > - Fixed build failure with !ZONED. > > - Fixed accidentally dropping BINARY_MOUNTDATA flag. > > - Added Reviewed-by's collected up to this point. > > > > These have run through our CI a few times, they haven't introduced any > > regressions. > > > > --- Original email --- > > Hello, > > > > These patches convert us to use the new mount API. Christian tried to do this a > > few months ago, but ran afoul of our preference to have a bunch of small > > changes. I started this series before I knew he had tried to convert us, so > > there's a fair bit that's different, but I did copy his approach for the remount > > bit. I've linked to the original patch where I took inspiration, Christian let > > me know if you want some other annotation for credit, I wasn't really sure the > > best way to do that. > > > > There are a few preparatory patches in the beginning, and then cleanups at the > > end. I took each call back one at a time to try and make it as small as > > possible. The resulting code is less, but the diffstat shows more insertions > > that deletions. This is because there are some big comment blocks around some > > of the more subtle things that we're doing to hopefully make it more clear. > > > > This is currently running through our CI. I thought it was fine last week but > > we had a bunch of new failures when I finished up the remount behavior. However > > today I discovered this was a regression in btrfs-progs, and I'm re-running the > > tests with the fixes. If anything major breaks in the CI I'll resend with > > fixes, but I'm pretty sure these patches will pass without issue. > > > > I utilized __maybe_unused liberally to make sure everything compiled while > > applied. The only "big" patch is where I went and removed the old API. If > > requested I can break that up a bit more, but I didn't think it was necessary. > > I did make sure to keep it in its own patch, so the switch to the new mount API > > path only has things we need to support the new mount API, and then the next > > patch removes the old code. Thanks, > > > > Josef > > > > Christian Brauner (1): > > fs: indicate request originates from old mount api > > > > Josef Bacik (18): > > btrfs: split out the mount option validation code into its own helper > > btrfs: set default compress type at btrfs_init_fs_info time > > btrfs: move space cache settings into open_ctree > > btrfs: do not allow free space tree rebuild on extent tree v2 > > btrfs: split out ro->rw and rw->ro helpers into their own functions > > btrfs: add a NOSPACECACHE mount option flag > > btrfs: add fs_parameter definitions > > btrfs: add parse_param callback for the new mount api > > btrfs: add fs context handling functions > > btrfs: add reconfigure callback for fs_context > > btrfs: add get_tree callback for new mount API > > btrfs: handle the ro->rw transition for mounting different subovls > > btrfs: switch to the new mount API > > btrfs: move the device specific mount options to super.c > > btrfs: remove old mount API code > > btrfs: move one shot mount option clearing to super.c > > btrfs: set clear_cache if we use usebackuproot > > btrfs: remove code for inode_cache and recovery mount options > > > > fs/btrfs/disk-io.c | 85 +- > > fs/btrfs/disk-io.h | 1 - > > fs/btrfs/fs.h | 15 +- > > fs/btrfs/super.c | 2357 +++++++++++++++++++++++--------------------- > > fs/btrfs/super.h | 5 +- > > fs/btrfs/zoned.c | 16 +- > > fs/btrfs/zoned.h | 6 +- > > fs/namespace.c | 11 + > > 8 files changed, 1263 insertions(+), 1233 deletions(-) > > > > -- > > 2.41.0 > > > > Looks like my r-b wasn't picked up for this revision, but looking over > it, things seem to be fine. Honestly Neal, I don't know how I should interpret your Reviewed-by. You don't contribute code or otherwise comment on other patches on the technical level. If you as an involved user want to give feedback that some feature is desired then it's fine to do so but the rev-by tag is not the way to do that. https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
On Wed, Nov 22, 2023 at 1:37 PM David Sterba <dsterba@suse.cz> wrote: > > On Wed, Nov 22, 2023 at 12:41:30PM -0500, Neal Gompa wrote: > > On Wed, Nov 22, 2023 at 12:18 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > v2->v3: > > > - Fixed up the various review comments from Dave and Anand. > > > - Added a patch to drop the deprecated mount options we currently have. > > > > > > v1->v2: > > > - Fixed up some nits and paste errors. > > > - Fixed build failure with !ZONED. > > > - Fixed accidentally dropping BINARY_MOUNTDATA flag. > > > - Added Reviewed-by's collected up to this point. > > > > > > These have run through our CI a few times, they haven't introduced any > > > regressions. > > > > > > --- Original email --- > > > Hello, > > > > > > These patches convert us to use the new mount API. Christian tried to do this a > > > few months ago, but ran afoul of our preference to have a bunch of small > > > changes. I started this series before I knew he had tried to convert us, so > > > there's a fair bit that's different, but I did copy his approach for the remount > > > bit. I've linked to the original patch where I took inspiration, Christian let > > > me know if you want some other annotation for credit, I wasn't really sure the > > > best way to do that. > > > > > > There are a few preparatory patches in the beginning, and then cleanups at the > > > end. I took each call back one at a time to try and make it as small as > > > possible. The resulting code is less, but the diffstat shows more insertions > > > that deletions. This is because there are some big comment blocks around some > > > of the more subtle things that we're doing to hopefully make it more clear. > > > > > > This is currently running through our CI. I thought it was fine last week but > > > we had a bunch of new failures when I finished up the remount behavior. However > > > today I discovered this was a regression in btrfs-progs, and I'm re-running the > > > tests with the fixes. If anything major breaks in the CI I'll resend with > > > fixes, but I'm pretty sure these patches will pass without issue. > > > > > > I utilized __maybe_unused liberally to make sure everything compiled while > > > applied. The only "big" patch is where I went and removed the old API. If > > > requested I can break that up a bit more, but I didn't think it was necessary. > > > I did make sure to keep it in its own patch, so the switch to the new mount API > > > path only has things we need to support the new mount API, and then the next > > > patch removes the old code. Thanks, > > > > > > Josef > > > > > > Christian Brauner (1): > > > fs: indicate request originates from old mount api > > > > > > Josef Bacik (18): > > > btrfs: split out the mount option validation code into its own helper > > > btrfs: set default compress type at btrfs_init_fs_info time > > > btrfs: move space cache settings into open_ctree > > > btrfs: do not allow free space tree rebuild on extent tree v2 > > > btrfs: split out ro->rw and rw->ro helpers into their own functions > > > btrfs: add a NOSPACECACHE mount option flag > > > btrfs: add fs_parameter definitions > > > btrfs: add parse_param callback for the new mount api > > > btrfs: add fs context handling functions > > > btrfs: add reconfigure callback for fs_context > > > btrfs: add get_tree callback for new mount API > > > btrfs: handle the ro->rw transition for mounting different subovls > > > btrfs: switch to the new mount API > > > btrfs: move the device specific mount options to super.c > > > btrfs: remove old mount API code > > > btrfs: move one shot mount option clearing to super.c > > > btrfs: set clear_cache if we use usebackuproot > > > btrfs: remove code for inode_cache and recovery mount options > > > > > > fs/btrfs/disk-io.c | 85 +- > > > fs/btrfs/disk-io.h | 1 - > > > fs/btrfs/fs.h | 15 +- > > > fs/btrfs/super.c | 2357 +++++++++++++++++++++++--------------------- > > > fs/btrfs/super.h | 5 +- > > > fs/btrfs/zoned.c | 16 +- > > > fs/btrfs/zoned.h | 6 +- > > > fs/namespace.c | 11 + > > > 8 files changed, 1263 insertions(+), 1233 deletions(-) > > > > > > -- > > > 2.41.0 > > > > > > > Looks like my r-b wasn't picked up for this revision, but looking over > > it, things seem to be fine. > > Honestly Neal, I don't know how I should interpret your Reviewed-by. You > don't contribute code or otherwise comment on other patches on the > technical level. If you as an involved user want to give feedback that > some feature is desired then it's fine to do so but the rev-by tag is > not the way to do that. > > https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes If it helps, I put "Reviewed-by" on patch sets that I have downloaded and applied to a local build that I test and read to check to see that they make sense. If the patches make sense, build, and work for me, I'll send a "Reviewed-by" statement. I figured that would be sufficient for "Reviewed-by" statements. If there's something more you're expecting for that, then please let me know. It's pretty rare that I'd test code without at least looking at it and checking it over, so I don't feel "Tested-by" makes sense.
On Wed, Nov 22, 2023 at 05:12:02PM -0500, Neal Gompa wrote: > On Wed, Nov 22, 2023 at 1:37 PM David Sterba <dsterba@suse.cz> wrote: > > > > On Wed, Nov 22, 2023 at 12:41:30PM -0500, Neal Gompa wrote: > > > On Wed, Nov 22, 2023 at 12:18 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > > > v2->v3: > > > > - Fixed up the various review comments from Dave and Anand. > > > > - Added a patch to drop the deprecated mount options we currently have. > > > > > > > > v1->v2: > > > > - Fixed up some nits and paste errors. > > > > - Fixed build failure with !ZONED. > > > > - Fixed accidentally dropping BINARY_MOUNTDATA flag. > > > > - Added Reviewed-by's collected up to this point. > > > > > > > > These have run through our CI a few times, they haven't introduced any > > > > regressions. > > > > > > > > --- Original email --- > > > > Hello, > > > > > > > > These patches convert us to use the new mount API. Christian tried to do this a > > > > few months ago, but ran afoul of our preference to have a bunch of small > > > > changes. I started this series before I knew he had tried to convert us, so > > > > there's a fair bit that's different, but I did copy his approach for the remount > > > > bit. I've linked to the original patch where I took inspiration, Christian let > > > > me know if you want some other annotation for credit, I wasn't really sure the > > > > best way to do that. > > > > > > > > There are a few preparatory patches in the beginning, and then cleanups at the > > > > end. I took each call back one at a time to try and make it as small as > > > > possible. The resulting code is less, but the diffstat shows more insertions > > > > that deletions. This is because there are some big comment blocks around some > > > > of the more subtle things that we're doing to hopefully make it more clear. > > > > > > > > This is currently running through our CI. I thought it was fine last week but > > > > we had a bunch of new failures when I finished up the remount behavior. However > > > > today I discovered this was a regression in btrfs-progs, and I'm re-running the > > > > tests with the fixes. If anything major breaks in the CI I'll resend with > > > > fixes, but I'm pretty sure these patches will pass without issue. > > > > > > > > I utilized __maybe_unused liberally to make sure everything compiled while > > > > applied. The only "big" patch is where I went and removed the old API. If > > > > requested I can break that up a bit more, but I didn't think it was necessary. > > > > I did make sure to keep it in its own patch, so the switch to the new mount API > > > > path only has things we need to support the new mount API, and then the next > > > > patch removes the old code. Thanks, > > > > > > > > Josef > > > > > > > > Christian Brauner (1): > > > > fs: indicate request originates from old mount api > > > > > > > > Josef Bacik (18): > > > > btrfs: split out the mount option validation code into its own helper > > > > btrfs: set default compress type at btrfs_init_fs_info time > > > > btrfs: move space cache settings into open_ctree > > > > btrfs: do not allow free space tree rebuild on extent tree v2 > > > > btrfs: split out ro->rw and rw->ro helpers into their own functions > > > > btrfs: add a NOSPACECACHE mount option flag > > > > btrfs: add fs_parameter definitions > > > > btrfs: add parse_param callback for the new mount api > > > > btrfs: add fs context handling functions > > > > btrfs: add reconfigure callback for fs_context > > > > btrfs: add get_tree callback for new mount API > > > > btrfs: handle the ro->rw transition for mounting different subovls > > > > btrfs: switch to the new mount API > > > > btrfs: move the device specific mount options to super.c > > > > btrfs: remove old mount API code > > > > btrfs: move one shot mount option clearing to super.c > > > > btrfs: set clear_cache if we use usebackuproot > > > > btrfs: remove code for inode_cache and recovery mount options > > > > > > > > fs/btrfs/disk-io.c | 85 +- > > > > fs/btrfs/disk-io.h | 1 - > > > > fs/btrfs/fs.h | 15 +- > > > > fs/btrfs/super.c | 2357 +++++++++++++++++++++++--------------------- > > > > fs/btrfs/super.h | 5 +- > > > > fs/btrfs/zoned.c | 16 +- > > > > fs/btrfs/zoned.h | 6 +- > > > > fs/namespace.c | 11 + > > > > 8 files changed, 1263 insertions(+), 1233 deletions(-) > > > > > > > > -- > > > > 2.41.0 > > > > > > > > > > Looks like my r-b wasn't picked up for this revision, but looking over > > > it, things seem to be fine. > > > > Honestly Neal, I don't know how I should interpret your Reviewed-by. You > > don't contribute code or otherwise comment on other patches on the > > technical level. If you as an involved user want to give feedback that > > some feature is desired then it's fine to do so but the rev-by tag is > > not the way to do that. > > > > https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > If it helps, I put "Reviewed-by" on patch sets that I have downloaded > and applied to a local build that I test and read to check to see that > they make sense. Download and build is just a build test, you may find build problems like with missing config options or warnings though which is valuable but not enough. Reading code if it makes sense means different things to you and me. I expect knowledge deep enough that if somebody puts a Reviewed-by there then I can ask questions to get explanations how things get fixed or improved. Or to CC that person when a bug appears and then jointly debug it or expect a fix. > If the patches make sense, build, and work for me, I'll send a > "Reviewed-by" statement. I figured that would be sufficient for > "Reviewed-by" statements. If there's something more you're expecting > for that, then please let me know. I expect that I can rely on the person's understanding of the code. It takes time to build the reputation, actually based on real code-level discussions or sending own patches of a reasonable quality. Otherwise it devalues the tag meaning. > It's pretty rare that I'd test code > without at least looking at it and checking it over, so I don't feel > "Tested-by" makes sense. If you test the code then it's for Tested-by, eventually stating which testsuites were run or in what configuration, which would apply in this case. I would be sad to discourage you from contributing, it happens on different levels, just that I was surprised first and was not the only one.
On Wed, Nov 22, 2023 at 11:55:00PM +0100, David Sterba wrote: > On Wed, Nov 22, 2023 at 05:12:02PM -0500, Neal Gompa wrote: > > On Wed, Nov 22, 2023 at 1:37 PM David Sterba <dsterba@suse.cz> wrote: > > > > > > On Wed, Nov 22, 2023 at 12:41:30PM -0500, Neal Gompa wrote: > > > > On Wed, Nov 22, 2023 at 12:18 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > > > > > v2->v3: > > > > > - Fixed up the various review comments from Dave and Anand. > > > > > - Added a patch to drop the deprecated mount options we currently have. > > > > > > > > > > v1->v2: > > > > > - Fixed up some nits and paste errors. > > > > > - Fixed build failure with !ZONED. > > > > > - Fixed accidentally dropping BINARY_MOUNTDATA flag. > > > > > - Added Reviewed-by's collected up to this point. > > > > > > > > > > These have run through our CI a few times, they haven't introduced any > > > > > regressions. > > > > > > > > > > --- Original email --- > > > > > Hello, > > > > > > > > > > These patches convert us to use the new mount API. Christian tried to do this a > > > > > few months ago, but ran afoul of our preference to have a bunch of small > > > > > changes. I started this series before I knew he had tried to convert us, so > > > > > there's a fair bit that's different, but I did copy his approach for the remount > > > > > bit. I've linked to the original patch where I took inspiration, Christian let > > > > > me know if you want some other annotation for credit, I wasn't really sure the > > > > > best way to do that. > > > > > > > > > > There are a few preparatory patches in the beginning, and then cleanups at the > > > > > end. I took each call back one at a time to try and make it as small as > > > > > possible. The resulting code is less, but the diffstat shows more insertions > > > > > that deletions. This is because there are some big comment blocks around some > > > > > of the more subtle things that we're doing to hopefully make it more clear. > > > > > > > > > > This is currently running through our CI. I thought it was fine last week but > > > > > we had a bunch of new failures when I finished up the remount behavior. However > > > > > today I discovered this was a regression in btrfs-progs, and I'm re-running the > > > > > tests with the fixes. If anything major breaks in the CI I'll resend with > > > > > fixes, but I'm pretty sure these patches will pass without issue. > > > > > > > > > > I utilized __maybe_unused liberally to make sure everything compiled while > > > > > applied. The only "big" patch is where I went and removed the old API. If > > > > > requested I can break that up a bit more, but I didn't think it was necessary. > > > > > I did make sure to keep it in its own patch, so the switch to the new mount API > > > > > path only has things we need to support the new mount API, and then the next > > > > > patch removes the old code. Thanks, > > > > > > > > > > Josef > > > > > > > > > > Christian Brauner (1): > > > > > fs: indicate request originates from old mount api > > > > > > > > > > Josef Bacik (18): > > > > > btrfs: split out the mount option validation code into its own helper > > > > > btrfs: set default compress type at btrfs_init_fs_info time > > > > > btrfs: move space cache settings into open_ctree > > > > > btrfs: do not allow free space tree rebuild on extent tree v2 > > > > > btrfs: split out ro->rw and rw->ro helpers into their own functions > > > > > btrfs: add a NOSPACECACHE mount option flag > > > > > btrfs: add fs_parameter definitions > > > > > btrfs: add parse_param callback for the new mount api > > > > > btrfs: add fs context handling functions > > > > > btrfs: add reconfigure callback for fs_context > > > > > btrfs: add get_tree callback for new mount API > > > > > btrfs: handle the ro->rw transition for mounting different subovls > > > > > btrfs: switch to the new mount API > > > > > btrfs: move the device specific mount options to super.c > > > > > btrfs: remove old mount API code > > > > > btrfs: move one shot mount option clearing to super.c > > > > > btrfs: set clear_cache if we use usebackuproot > > > > > btrfs: remove code for inode_cache and recovery mount options > > > > > > > > > > fs/btrfs/disk-io.c | 85 +- > > > > > fs/btrfs/disk-io.h | 1 - > > > > > fs/btrfs/fs.h | 15 +- > > > > > fs/btrfs/super.c | 2357 +++++++++++++++++++++++--------------------- > > > > > fs/btrfs/super.h | 5 +- > > > > > fs/btrfs/zoned.c | 16 +- > > > > > fs/btrfs/zoned.h | 6 +- > > > > > fs/namespace.c | 11 + > > > > > 8 files changed, 1263 insertions(+), 1233 deletions(-) > > > > > > > > > > -- > > > > > 2.41.0 > > > > > > > > > > > > > Looks like my r-b wasn't picked up for this revision, but looking over > > > > it, things seem to be fine. > > > > > > Honestly Neal, I don't know how I should interpret your Reviewed-by. You > > > don't contribute code or otherwise comment on other patches on the > > > technical level. If you as an involved user want to give feedback that > > > some feature is desired then it's fine to do so but the rev-by tag is > > > not the way to do that. > > > > > > https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > > If it helps, I put "Reviewed-by" on patch sets that I have downloaded > > and applied to a local build that I test and read to check to see that > > they make sense. > > Download and build is just a build test, you may find build problems > like with missing config options or warnings though which is valuable > but not enough. Reading code if it makes sense means different things to > you and me. I expect knowledge deep enough that if somebody puts a > Reviewed-by there then I can ask questions to get explanations how > things get fixed or improved. Or to CC that person when a bug appears > and then jointly debug it or expect a fix. > > > If the patches make sense, build, and work for me, I'll send a > > "Reviewed-by" statement. I figured that would be sufficient for > > "Reviewed-by" statements. If there's something more you're expecting > > for that, then please let me know. > > I expect that I can rely on the person's understanding of the code. It > takes time to build the reputation, actually based on real code-level > discussions or sending own patches of a reasonable quality. Otherwise it > devalues the tag meaning. > > > It's pretty rare that I'd test code > > without at least looking at it and checking it over, so I don't feel > > "Tested-by" makes sense. > > If you test the code then it's for Tested-by, eventually stating which > testsuites were run or in what configuration, which would apply in this > case. > > I would be sad to discourage you from contributing, it happens on > different levels, just that I was surprised first and was not the only > one. Generally speaking we all have different ideas of what the 'Reviewed-by' tag means. Dave your definition is a relatively common one, and isn't incorrect. However we as a kernel community have been trying to encourage more participation and have specifically called out patch review as a good place to start. Everybody has something to add, and in Neal's case specifically he's been working in storage/file systems for many years. He and Chris Murphy were the primary drivers of btrfs adoption in Fedora. I've been fielding bug and feature requests from him for years. Is he a veteran kernel developer? No. Does he have context on btrfs and it's wider usage and how it behaves? Absolutely. He's able to add context to use cases that we may not think about from the user side https://lore.kernel.org/linux-btrfs/CAEg-Je_zGBAgPLgpnjWbRwGLXNSpmor-mokZyMT6iSfF2121QQ@mail.gmail.com/ https://lore.kernel.org/linux-btrfs/CAEg-Je_b1YtdsCR0zS5XZ_SbvJgN70ezwvRwLiCZgDGLbeMB=w@mail.gmail.com/ Additionally as the btrfs-progs maintainer for Fedora he's testing and rolling our packages regularly, and is able to provide feedback and patches https://lore.kernel.org/linux-btrfs/CAEg-Je8FNefaKjYFeqw2Gh-TXSjVWbgczYtL3qpgJWQj3pJjCw@mail.gmail.com/ https://lore.kernel.org/linux-btrfs/20230920140635.2066172-2-neal@gompa.dev/ In addition to just generally testing the things that we ship, even big things like the sub-page blocksize stuff that I know he's been involved in heavily because of the Fedora-Asahi stuff https://lore.kernel.org/linux-btrfs/CAEg-Je8b74AHC32Z7Zwrfz_83bBSLjaU58AdzvgSanBNoZ3X=w@mail.gmail.com/ I don't think anybody here is arguing that Neal doesn't provide value to the community, but I wanted to provide context for random passer-by's to understand the full context. I personally value feedback from Neal. I regularly get feedback from the larger Fedora community from him and others and use that as a way to guide our development and feature work. He's also usually the first one to raise important user-facing bugs we miss, the most recent one being the -EFAULT in the O_DIRECT write case that Boris fixed earlier this year. I know if he's looked at my patches and he doesn't understand something that I didn't do a good job of explaining my changes. Looking through the history of Reviewed-by's from Neal, and then looking at what has been committed there's a pretty big discrepancy. There's 4 patches with his Reviewed-by in the git tree, but a lot more series that he's reviewed. It is fair to say "this is our standard for Reviewed-by, you need to do X for us to attribute value to Reviewed-by." It's not fair to silently ignore and drop tags, especially for a contributor we know is actively involved. Neal isn't trying to score internet points, he's trying to contribute. Looking through our process documentation at the different tags it looks like the 'Acked-by' tag is a reasonable middle-ground for this type of contribution ``` If a person was not directly involved in the preparation or handling of a patch but wishes to signify and record their approval of it then they can ask to have an Acked-by: line added to the patch's changelog. ``` I don't want to open up for a flood of random people to try and score internet points, so I don't think we should allow anybody to get their 'Acked-by's on any patchset, but in the case of contributors like Neal and others I think this is a good way to acknowledge their contributions. I would propose that we use 'Acked-by' for contributors who have been in the community and active for a while and that have context of the wider use of btrfs, but not necessarily code contributions that would indicate deep understanding of the relevant code. I'm fine with using Dave's definition for 'Reviewed-by', so long as we communicate to new contributors our expectations of what these tags mean. As maintainers of btrfs we need to be better about communicating our expectations to new contributors. Guiding new people is an important part of keeping a thriving community. I will be more proactive about this as it's an important area for me, but this is not just a thing that needs to be a me, Dave, or Chris's responsibility, every core contributor to btrfs is capable of doing this sort of guidance. Going forward, is using 'Acked-by' an acceptable compromise for everybody in these cases? Are we satisfied with the standards I've laid out above? Thanks, Josef
On Wed, Nov 22, 2023 at 12:17:36PM -0500, Josef Bacik wrote: > v2->v3: > - Fixed up the various review comments from Dave and Anand. > - Added a patch to drop the deprecated mount options we currently have. I finished review of v3, there were some changes missing from my v2 comments, I also did some renames and comment updates. Patches moved from topic branch to misc-next, thanks.
On 29/11/2023 05:15, David Sterba wrote: > On Wed, Nov 22, 2023 at 12:17:36PM -0500, Josef Bacik wrote: >> v2->v3: >> - Fixed up the various review comments from Dave and Anand. >> - Added a patch to drop the deprecated mount options we currently have. > > I finished review of v3, there were some changes missing from my v2 > comments, I also did some renames and comment updates. Patches moved > from topic branch to misc-next, thanks. Apologies for the delayed review. The renaming of check_options() should have been in patch 2/19 instead of patch 14/19, avoids confusion in the function stack. For now, except for the patches listed here [1], the remaining ones have been reviewed. Reviewed-by: Anand Jain <anand.jain@oracle.com> fs: indicate request originates from old mount api btrfs: split out the mount option validation code into its own helper btrfs: set default compress type at btrfs_init_fs_info time btrfs: move space cache settings into open_ctree btrfs: do not allow free space tree rebuild on extent tree v2 btrfs: split out ro->rw and rw->ro helpers into their own functions btrfs: add a NOSPACECACHE mount option flag btrfs: add fs_parameter definitions [1] { btrfs: add parse_param callback for the new mount api btrfs: add fs context handling functions btrfs: add reconfigure callback for fs_context btrfs: add get_tree callback for new mount API btrfs: handle the ro->rw transition for mounting different subovls btrfs: switch to the new mount API } btrfs: move the device specific mount options to super.c btrfs: remove old mount API code btrfs: move one shot mount option clearing to super.c btrfs: set clear_cache if we use usebackuproot btrfs: remove code for inode_cache and recovery mount options
On Wed, Nov 29, 2023 at 08:59:57PM +0800, Anand Jain wrote: > On 29/11/2023 05:15, David Sterba wrote: > > On Wed, Nov 22, 2023 at 12:17:36PM -0500, Josef Bacik wrote: > >> v2->v3: > >> - Fixed up the various review comments from Dave and Anand. > >> - Added a patch to drop the deprecated mount options we currently have. > > > > I finished review of v3, there were some changes missing from my v2 > > comments, I also did some renames and comment updates. Patches moved > > from topic branch to misc-next, thanks. > Apologies for the delayed review. > > The renaming of check_options() should have been in patch 2/19 > instead of patch 14/19, avoids confusion in the function stack. Yeah it could have been renamed in patch 2, though I hope it's not a big deal. The final name is with the btrfs_ prefix so the stack name confusion would happen only when debugging something between patches 2 and 14. > For now, except for the patches listed here [1], > the remaining ones have been reviewed. Thanks. In case you'd continue with the remaining patches feel free to send rev-by later.