Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 2018-05-18 04:06, Anand Jain wrote: Thanks Austin and Jeff for the suggestion. I am not particularly a fan of mount option either mainly because those options aren't persistent and host independent luns will have tough time to have them synchronize manually. Properties are better as it is persistent. And we can apply this read_mirror_policy property on the fsid object. But if we are talking about the properties then it can be stored as extended attributes or ondisk key value pair, and I am doubt if ondisk key value pair will get a nod. I can explore the extended attribute approach but appreciate more comments. Hmm, thinking a bit further, might it be easier to just keep this as a mount option, and add something that lets you embed default mount options in the volume in a free-form manner? Then, you could set this persistently there, and could specify any others you want too. Doing that would also give very well defined behavior for exactly when changes would apply (the next time you mount or remount the volume), though handling of whether or not an option came from there or was specified on the command-line might be a bit complicated. On 05/17/2018 10:46 PM, Jeff Mahoney wrote: On 5/17/18 8:25 AM, Austin S. Hemmelgarn wrote: On 2018-05-16 22:32, Anand Jain wrote: On 05/17/2018 06:35 AM, David Sterba wrote: On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. I am ok to make it ioctl for the final. What do you think? But to reproduce the bug posted in Btrfs: fix the corruption by reading stale btree blocks It needs to be a mount option, as randomly the pid can still pick the disk specified in the mount option. Personally, I'd vote for filesystem property (thus handled through the standard `btrfs property` command) that can be overridden by a mount option. With that approach, no new tool (or change to an existing tool) would be needed, existing volumes could be converted to use it in a backwards compatible manner (old kernels would just ignore the property), and you could still have the behavior you want in tests (and in theory it could easily be adapted to be a per-subvolume setting if we ever get per-subvolume chunk profile support). Properties are a combination of interfaces presented through a single command. Although the kernel API would allow a direct-to-property interface via the btrfs.* extended attributes, those are currently limited to a single inode. The label property is set via ioctl and stored in the superblock. The read-only subvolume property is also set by ioctl but stored in the root flags. As it stands, every property is explicitly defined in the tools, so any addition would require tools changes. This is a bigger discussion, though. We *could* use the xattr interface to access per-root or fs-global properties, but we'd need to define that interface. btrfs_listxattr could get interesting, though I suppose we could simplify it by only allowing the per-subvolume and fs-global operations on root inodes. -Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] btrfs: add read mirror policy
Thanks Austin and Jeff for the suggestion. I am not particularly a fan of mount option either mainly because those options aren't persistent and host independent luns will have tough time to have them synchronize manually. Properties are better as it is persistent. And we can apply this read_mirror_policy property on the fsid object. But if we are talking about the properties then it can be stored as extended attributes or ondisk key value pair, and I am doubt if ondisk key value pair will get a nod. I can explore the extended attribute approach but appreciate more comments. -Anand On 05/17/2018 10:46 PM, Jeff Mahoney wrote: On 5/17/18 8:25 AM, Austin S. Hemmelgarn wrote: On 2018-05-16 22:32, Anand Jain wrote: On 05/17/2018 06:35 AM, David Sterba wrote: On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. I am ok to make it ioctl for the final. What do you think? But to reproduce the bug posted in Btrfs: fix the corruption by reading stale btree blocks It needs to be a mount option, as randomly the pid can still pick the disk specified in the mount option. Personally, I'd vote for filesystem property (thus handled through the standard `btrfs property` command) that can be overridden by a mount option. With that approach, no new tool (or change to an existing tool) would be needed, existing volumes could be converted to use it in a backwards compatible manner (old kernels would just ignore the property), and you could still have the behavior you want in tests (and in theory it could easily be adapted to be a per-subvolume setting if we ever get per-subvolume chunk profile support). Properties are a combination of interfaces presented through a single command. Although the kernel API would allow a direct-to-property interface via the btrfs.* extended attributes, those are currently limited to a single inode. The label property is set via ioctl and stored in the superblock. The read-only subvolume property is also set by ioctl but stored in the root flags. As it stands, every property is explicitly defined in the tools, so any addition would require tools changes. This is a bigger discussion, though. We *could* use the xattr interface to access per-root or fs-global properties, but we'd need to define that interface. btrfs_listxattr could get interesting, though I suppose we could simplify it by only allowing the per-subvolume and fs-global operations on root inodes. -Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 2018-05-17 10:46, Jeff Mahoney wrote: On 5/16/18 6:35 PM, David Sterba wrote: On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. I've seen a few alternate suggestions in the thread. I suppose the real question is: what and where is the intended persistence for this choice? A mount option gets it via fstab. How would a user be expected to set it consistently via ioctl on each mount? Properties could work, but there's more discussion needed there. Personally, I like the property idea since it could conceivably be used on a per-file basis. For the specific proposed use case (the tests), it probably doesn't need to be persistent beyond mount options. However, this also allows for a trivial configuration using a slow storage device to provide redundancy for a fast storage device of the same size, which is potentially very useful for some people. In that case, I can see most people who would be using it wanting it to follow the filesystem regardless of what context it's being mounted in (for example, it shouldn't need an extra option if mounted from a recovery environment or if it's moved to another system). Most of my reason for recommending properties is that filesystem level properties appear to be the best thing BTRFS has to store per-volume configuration that's supposed to stay with the volume, despite not really being used for that even though there are quite a few mount options that are logical candidates for this type of thing (for example, the `ssd` options, `metadata_ratio`, and `max_inline` all make more logical sense as a property of the volume, not the mount). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 5/17/18 8:25 AM, Austin S. Hemmelgarn wrote: > On 2018-05-16 22:32, Anand Jain wrote: >> >> >> On 05/17/2018 06:35 AM, David Sterba wrote: >>> On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- >>> >>> Mount option is mostly likely not the right interface for setting such >>> options, as usual. >> >> I am ok to make it ioctl for the final. What do you think? >> >> >> But to reproduce the bug posted in >> Btrfs: fix the corruption by reading stale btree blocks >> It needs to be a mount option, as randomly the pid can >> still pick the disk specified in the mount option. >> > Personally, I'd vote for filesystem property (thus handled through the > standard `btrfs property` command) that can be overridden by a mount > option. With that approach, no new tool (or change to an existing tool) > would be needed, existing volumes could be converted to use it in a > backwards compatible manner (old kernels would just ignore the > property), and you could still have the behavior you want in tests (and > in theory it could easily be adapted to be a per-subvolume setting if we > ever get per-subvolume chunk profile support). Properties are a combination of interfaces presented through a single command. Although the kernel API would allow a direct-to-property interface via the btrfs.* extended attributes, those are currently limited to a single inode. The label property is set via ioctl and stored in the superblock. The read-only subvolume property is also set by ioctl but stored in the root flags. As it stands, every property is explicitly defined in the tools, so any addition would require tools changes. This is a bigger discussion, though. We *could* use the xattr interface to access per-root or fs-global properties, but we'd need to define that interface. btrfs_listxattr could get interesting, though I suppose we could simplify it by only allowing the per-subvolume and fs-global operations on root inodes. -Jeff -- Jeff Mahoney SUSE Labs signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 5/16/18 6:35 PM, David Sterba wrote: > On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: >> Not yet ready for the integration. As I need to introduce >> -o no_read_mirror_policy instead of -o read_mirror_policy=- > > Mount option is mostly likely not the right interface for setting such > options, as usual. I've seen a few alternate suggestions in the thread. I suppose the real question is: what and where is the intended persistence for this choice? A mount option gets it via fstab. How would a user be expected to set it consistently via ioctl on each mount? Properties could work, but there's more discussion needed there. Personally, I like the property idea since it could conceivably be used on a per-file basis. -Jeff -- Jeff Mahoney SUSE Labs signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 2018-05-16 22:32, Anand Jain wrote: On 05/17/2018 06:35 AM, David Sterba wrote: On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. I am ok to make it ioctl for the final. What do you think? But to reproduce the bug posted in Btrfs: fix the corruption by reading stale btree blocks It needs to be a mount option, as randomly the pid can still pick the disk specified in the mount option. Personally, I'd vote for filesystem property (thus handled through the standard `btrfs property` command) that can be overridden by a mount option. With that approach, no new tool (or change to an existing tool) would be needed, existing volumes could be converted to use it in a backwards compatible manner (old kernels would just ignore the property), and you could still have the behavior you want in tests (and in theory it could easily be adapted to be a per-subvolume setting if we ever get per-subvolume chunk profile support). Of course, I'd actually like to see most of the mount options available as filesystem level properties with the option to override through mount options, but that's a lot more ambitious of an undertaking. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On 05/17/2018 06:35 AM, David Sterba wrote: On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. I am ok to make it ioctl for the final. What do you think? But to reproduce the bug posted in Btrfs: fix the corruption by reading stale btree blocks It needs to be a mount option, as randomly the pid can still pick the disk specified in the mount option. -Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] btrfs: add read mirror policy
On Wed, May 16, 2018 at 06:03:56PM +0800, Anand Jain wrote: > Not yet ready for the integration. As I need to introduce > -o no_read_mirror_policy instead of -o read_mirror_policy=- Mount option is mostly likely not the right interface for setting such options, as usual. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] btrfs: add read mirror policy
Not yet ready for the integration. As I need to introduce -o no_read_mirror_policy instead of -o read_mirror_policy=- to reset the policy as in 3/3. But I am sending this early so that we can use it for btrfs/161 in the ML, and this patch-set is stable enough for the testing. Anand Jain (3): btrfs: add mount option read_mirror_policy btrfs: add read_mirror_policy parameter devid btrfs: read_mirror_policy ability to reset fs/btrfs/ctree.h | 2 ++ fs/btrfs/super.c | 44 fs/btrfs/volumes.c | 18 +- fs/btrfs/volumes.h | 7 +++ 4 files changed, 70 insertions(+), 1 deletion(-) -- 2.7.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
