| Message ID | cover.1719257716.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add the ability to query mount options in statmount | expand |
On Mon, 2024-06-24 at 15:40 -0400, Josef Bacik wrote: > Hello, > > Currently if you want to get mount options for a mount and you're > using > statmount(), you still have to open /proc/mounts to parse the mount > options. > statmount() does have the ability to store an arbitrary string > however, > additionally the way we do that is with a seq_file, which is also how > we use > ->show_options for the individual file systems. > > Extent statmount() to have a flag for fetching the mount options of a > mount. > This allows users to not have to parse /proc mount for anything > related to a > mount. I've extended the existing statmount() test to validate this > feature > works as expected. As you can tell from the ridiculous amount of > silly string > parsing, this is a huge win for users and climate change as we will > no longer > have to waste several cycles parsing strings anymore. > > This is based on my branch that extends listmount/statmount to walk > into foreign > namespaces. Below are links to that posting, that branch, and this > branch to > make it easier to review. > > https://lore.kernel.org/linux-fsdevel/cover.1719243756.git.josef@toxicpanda.com/ > https://github.com/josefbacik/linux/tree/listmount.combined > https://github.com/josefbacik/linux/tree/statmount-opts > > Thanks, > > Josef > > Josef Bacik (4): > fs: rename show_mnt_opts -> show_vfsmnt_opts > fs: add a helper to show all the options for a mount > fs: export mount options via statmount() > sefltests: extend the statmount test for mount options > > fs/internal.h | 5 + > fs/namespace.c | 7 + > fs/proc_namespace.c | 29 ++-- > include/uapi/linux/mount.h | 3 +- > .../filesystems/statmount/statmount_test.c | 131 > +++++++++++++++++- > 5 files changed, 164 insertions(+), 11 deletions(-) > Nice work. I especially like that there is a selftest now. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, Jun 24, 2024 at 03:40:49PM GMT, Josef Bacik wrote: > Hello, > > Currently if you want to get mount options for a mount and you're using > statmount(), you still have to open /proc/mounts to parse the mount options. > statmount() does have the ability to store an arbitrary string however, > additionally the way we do that is with a seq_file, which is also how we use > ->show_options for the individual file systems. > > Extent statmount() to have a flag for fetching the mount options of a mount. > This allows users to not have to parse /proc mount for anything related to a > mount. I've extended the existing statmount() test to validate this feature > works as expected. As you can tell from the ridiculous amount of silly string > parsing, this is a huge win for users and climate change as we will no longer > have to waste several cycles parsing strings anymore. > > This is based on my branch that extends listmount/statmount to walk into foreign > namespaces. Below are links to that posting, that branch, and this branch to > make it easier to review. So I was very hesitant to do it this way because I feel this is pretty ugly dumping mount options like that but Karel and others have the same use-case that they want to retrieve it all via statmount() (or another ID-based system call) so I guess I'll live with this. But note that this will be a fairly ugly interface at times. For example, mounting overlayfs with 500 lower layers then what one gets is: /mnt/test mnt_ns_id: 2 mnt_id: 4294969217 mnt_id_old: 820 mnt_parent_id: 4294967529 mnt_opts: rw,relatime,lowerdir+=/mnt/tmp1, lowerdir+=/mnt/tmp2,lowerdir+=/mnt/tmp3,lowerdir+=/mnt/tmp4,lowerdir+=/mnt/tmp5, lowerdir+=/mnt/tmp6,lowerdir+=/mnt/tmp7,lowerdir+=/mnt/tmp8,lowerdir+=/mnt/tmp9, lowerdir+=/mnt/tmp10,lowerdir+=/mnt/tmp11,lowerdir+=/mnt/tmp12, lowerdir+=/mnt/tmp13,lowerdir+=/mnt/tmp14,lowerdir+=/mnt/tmp15, lowerdir+=/mnt/tmp16,lowerdir+=/mnt/tmp17,lowerdir+=/mnt/tmp18, lowerdir+=/mnt/tmp19,lowerdir+=/mnt/tmp20,lowerdir+=/mnt/tmp21, lowerdir+=/mnt/tmp22,lowerdir+=/mnt/tmp23,lowerdir+=/mnt/tmp24, lowerdir+=/mnt/tmp25,lowerdir+=/mnt/tmp26,lowerdir+=/mnt/tmp27, lowerdir+=/mnt/tmp28,lowerdir+=/mnt/tmp29,lowerdir+=/mnt/tmp30, lowerdir+=/mnt/tmp31,lowerdir+=/mnt/tmp32,lowerdir+=/mnt/tmp33, lowerdir+=/mnt/tmp34,lowerdir+=/mnt/tmp35,lowerdir+=/mnt/tmp36, lowerdir+=/mnt/tmp37,lowerdir+=/mnt/tmp38,lowerdir+=/mnt/tmp39, lowerdir+=/mnt/tmp40,lowerdir+=/mnt/tmp41,lowerdir+=/mnt/tmp42, lowerdir+=/mnt/tmp43,lowerdir+=/mnt/tmp44,lowerdir+=/mnt/tmp45, lowerdir+=/mnt/tmp46,lowerdir+=/mnt/tmp47,lowerdir+=/mnt/tmp48, lowerdir+=/mnt/tmp49,lowerdir+=/mnt/tmp50,lowerdir+=/mnt/tmp51, lowerdir+=/mnt/tmp52,lowerdir+=/mnt/tmp53,lowerdir+=/mnt/tmp54, lowerdir+=/mnt/tmp55,lowerdir+=/mnt/tmp56,lowerdir+=/mnt/tmp57, lowerdir+=/mnt/tmp58,lowerdir+=/mnt/tmp59,lowerdir+=/mnt/tmp60, lowerdir+=/mnt/tmp61,lowerdir+=/mnt/tmp62,lowerdir+=/mnt/tmp63, lowerdir+=/mnt/tmp64,lowerdir+=/mnt/tmp65,lowerdir+=/mnt/tmp66, lowerdir+=/mnt/tmp67,lowerdir+=/mnt/tmp68,lowerdir+=/mnt/tmp69, lowerdir+=/mnt/tmp70,lowerdir+=/mnt/tmp71,lowerdir+=/mnt/tmp72, lowerdir+=/mnt/tmp73,lowerdir+=/mnt/tmp74,lowerdir+=/mnt/tmp75, lowerdir+=/mnt/tmp76,lowerdir+=/mnt/tmp77,lowerdir+=/mnt/tmp78, lowerdir+=/mnt/tmp79,lowerdir+=/mnt/tmp80,lowerdir+=/mnt/tmp81, lowerdir+=/mnt/tmp82,lowerdir+=/mnt/tmp83,lowerdir+=/mnt/tmp84, lowerdir+=/mnt/tmp85,lowerdir+=/mnt/tmp86,lowerdir+=/mnt/tmp87, lowerdir+=/mnt/tmp88,lowerdir+=/mnt/tmp89,lowerdir+=/mnt/tmp90, lowerdir+=/mnt/tmp91,lowerdir+=/mnt/tmp92,lowerdir+=/mnt/tmp93, lowerdir+=/mnt/tmp94,lowerdir+=/mnt/tmp95,lowerdir+=/mnt/tmp96, lowerdir+=/mnt/tmp97,lowerdir+=/mnt/tmp98,lowerdir+=/mnt/tmp99, lowerdir+=/mnt/tmp100,lowerdir+=/mnt/tmp101,lowerdir+=/mnt/tmp102, lowerdir+=/mnt/tmp103,lowerdir+=/mnt/tmp104,lowerdir+=/mnt/tmp105, lowerdir+=/mnt/tmp106,lowerdir+=/mnt/tmp107,lowerdir+=/mnt/tmp108, lowerdir+=/mnt/tmp109,lowerdir+=/mnt/tmp110,lowerdir+=/mnt/tmp111, lowerdir+=/mnt/tmp112,lowerdir+=/mnt/tmp113,lowerdir+=/mnt/tmp114, lowerdir+=/mnt/tmp115,lowerdir+=/mnt/tmp116,lowerdir+=/mnt/tmp117, lowerdir+=/mnt/tmp118,lowerdir+=/mnt/tmp119,lowerdir+=/mnt/tmp120, lowerdir+=/mnt/tmp121,lowerdir+=/mnt/tmp122,lowerdir+=/mnt/tmp123, lowerdir+=/mnt/tmp124,lowerdir+=/mnt/tmp125,lowerdir+=/mnt/tmp126, lowerdir+=/mnt/tmp127,lowerdir+=/mnt/tmp128,lowerdir+=/mnt/tmp129, lowerdir+=/mnt/tmp130,lowerdir+=/mnt/tmp131,lowerdir+=/mnt/tmp132, lowerdir+=/mnt/tmp133,lowerdir+=/mnt/tmp134,lowerdir+=/mnt/tmp135, lowerdir+=/mnt/tmp136,lowerdir+=/mnt/tmp137,lowerdir+=/mnt/tmp138, lowerdir+=/mnt/tmp139,lowerdir+=/mnt/tmp140,lowerdir+=/mnt/tmp141, lowerdir+=/mnt/tmp142,lowerdir+=/mnt/tmp143,lowerdir+=/mnt/tmp144, lowerdir+=/mnt/tmp145,lowerdir+=/mnt/tmp146,lowerdir+=/mnt/tmp147, lowerdir+=/mnt/tmp148,lowerdir+=/mnt/tmp149,lowerdir+=/mnt/tmp150, lowerdir+=/mnt/tmp151,lowerdir+=/mnt/tmp152,lowerdir+=/mnt/tmp153, lowerdir+=/mnt/tmp154,lowerdir+=/mnt/tmp155,lowerdir+=/mnt/tmp156, lowerdir+=/mnt/tmp157,lowerdir+=/mnt/tmp158,lowerdir+=/mnt/tmp159, lowerdir+=/mnt/tmp160,lowerdir+=/mnt/tmp161,lowerdir+=/mnt/tmp162, lowerdir+=/mnt/tmp163,lowerdir+=/mnt/tmp164,lowerdir+=/mnt/tmp165, lowerdir+=/mnt/tmp166,lowerdir+=/mnt/tmp167,lowerdir+=/mnt/tmp168, lowerdir+=/mnt/tmp169,lowerdir+=/mnt/tmp170,lowerdir+=/mnt/tmp171, lowerdir+=/mnt/tmp172,lowerdir+=/mnt/tmp173,lowerdir+=/mnt/tmp174, lowerdir+=/mnt/tmp175,lowerdir+=/mnt/tmp176,lowerdir+=/mnt/tmp177, lowerdir+=/mnt/tmp178,lowerdir+=/mnt/tmp179,lowerdir+=/mnt/tmp180, lowerdir+=/mnt/tmp181,lowerdir+=/mnt/tmp182,lowerdir+=/mnt/tmp183, lowerdir+=/mnt/tmp184,lowerdir+=/mnt/tmp185,lowerdir+=/mnt/tmp186, lowerdir+=/mnt/tmp187,lowerdir+=/mnt/tmp188,lowerdir+=/mnt/tmp189, lowerdir+=/mnt/tmp190,lowerdir+=/mnt/tmp191,lowerdir+=/mnt/tmp192, lowerdir+=/mnt/tmp193,lowerdir+=/mnt/tmp194,lowerdir+=/mnt/tmp195, lowerdir+=/mnt/tmp196,lowerdir+=/mnt/tmp197,lowerdir+=/mnt/tmp198, lowerdir+=/mnt/tmp199,lowerdir+=/mnt/tmp200,lowerdir+=/mnt/tmp201, lowerdir+=/mnt/tmp202,lowerdir+=/mnt/tmp203,lowerdir+=/mnt/tmp204, lowerdir+=/mnt/tmp205,lowerdir+=/mnt/tmp206,lowerdir+=/mnt/tmp207, lowerdir+=/mnt/tmp208,lowerdir+=/mnt/tmp209,lowerdir+=/mnt/tmp210, lowerdir+=/mnt/tmp211,lowerdir+=/mnt/tmp212,lowerdir+=/mnt/tmp213, lowerdir+=/mnt/tmp214,lowerdir+=/mnt/tmp215,lowerdir+=/mnt/tmp216, lowerdir+=/mnt/tmp217,lowerdir+=/mnt/tmp218,lowerdir+=/mnt/tmp219, lowerdir+=/mnt/tmp220,lowerdir+=/mnt/tmp221,lowerdir+=/mnt/tmp222, lowerdir+=/mnt/tmp223,lowerdir+=/mnt/tmp224,lowerdir+=/mnt/tmp225, lowerdir+=/mnt/tmp226,lowerdir+=/mnt/tmp227,lowerdir+=/mnt/tmp228, lowerdir+=/mnt/tmp229,lowerdir+=/mnt/tmp230,lowerdir+=/mnt/tmp231, lowerdir+=/mnt/tmp232,lowerdir+=/mnt/tmp233,lowerdir+=/mnt/tmp234, lowerdir+=/mnt/tmp235,lowerdir+=/mnt/tmp236,lowerdir+=/mnt/tmp237, lowerdir+=/mnt/tmp238,lowerdir+=/mnt/tmp239,lowerdir+=/mnt/tmp240, lowerdir+=/mnt/tmp241,lowerdir+=/mnt/tmp242,lowerdir+=/mnt/tmp243, lowerdir+=/mnt/tmp244,lowerdir+=/mnt/tmp245,lowerdir+=/mnt/tmp246, lowerdir+=/mnt/tmp247,lowerdir+=/mnt/tmp248,lowerdir+=/mnt/tmp249, lowerdir+=/mnt/tmp250,lowerdir+=/mnt/tmp251,lowerdir+=/mnt/tmp252, lowerdir+=/mnt/tmp253,lowerdir+=/mnt/tmp254,lowerdir+=/mnt/tmp255, lowerdir+=/mnt/tmp256,lowerdir+=/mnt/tmp257,lowerdir+=/mnt/tmp258, lowerdir+=/mnt/tmp259,lowerdir+=/mnt/tmp260,lowerdir+=/mnt/tmp261, lowerdir+=/mnt/tmp262,lowerdir+=/mnt/tmp263,lowerdir+=/mnt/tmp264, lowerdir+=/mnt/tmp265,lowerdir+=/mnt/tmp266,lowerdir+=/mnt/tmp267, lowerdir+=/mnt/tmp268,lowerdir+=/mnt/tmp269,lowerdir+=/mnt/tmp270, lowerdir+=/mnt/tmp271,lowerdir+=/mnt/tmp272,lowerdir+=/mnt/tmp273, lowerdir+=/mnt/tmp274,lowerdir+=/mnt/tmp275,lowerdir+=/mnt/tmp276, lowerdir+=/mnt/tmp277,lowerdir+=/mnt/tmp278,lowerdir+=/mnt/tmp279, lowerdir+=/mnt/tmp280,lowerdir+=/mnt/tmp281,lowerdir+=/mnt/tmp282, lowerdir+=/mnt/tmp283,lowerdir+=/mnt/tmp284,lowerdir+=/mnt/tmp285, lowerdir+=/mnt/tmp286,lowerdir+=/mnt/tmp287,lowerdir+=/mnt/tmp288, lowerdir+=/mnt/tmp289,lowerdir+=/mnt/tmp290,lowerdir+=/mnt/tmp291, lowerdir+=/mnt/tmp292,lowerdir+=/mnt/tmp293,lowerdir+=/mnt/tmp294, lowerdir+=/mnt/tmp295,lowerdir+=/mnt/tmp296,lowerdir+=/mnt/tmp297, lowerdir+=/mnt/tmp298,lowerdir+=/mnt/tmp299,lowerdir+=/mnt/tmp300, lowerdir+=/mnt/tmp301,lowerdir+=/mnt/tmp302,lowerdir+=/mnt/tmp303, lowerdir+=/mnt/tmp304,lowerdir+=/mnt/tmp305,lowerdir+=/mnt/tmp306, lowerdir+=/mnt/tmp307,lowerdir+=/mnt/tmp308,lowerdir+=/mnt/tmp309, lowerdir+=/mnt/tmp310,lowerdir+=/mnt/tmp311,lowerdir+=/mnt/tmp312, lowerdir+=/mnt/tmp313,lowerdir+=/mnt/tmp314,lowerdir+=/mnt/tmp315, lowerdir+=/mnt/tmp316,lowerdir+=/mnt/tmp317,lowerdir+=/mnt/tmp318, lowerdir+=/mnt/tmp319,lowerdir+=/mnt/tmp320,lowerdir+=/mnt/tmp321, lowerdir+=/mnt/tmp322,lowerdir+=/mnt/tmp323,lowerdir+=/mnt/tmp324, lowerdir+=/mnt/tmp325,lowerdir+=/mnt/tmp326,lowerdir+=/mnt/tmp327, lowerdir+=/mnt/tmp328,lowerdir+=/mnt/tmp329,lowerdir+=/mnt/tmp330, lowerdir+=/mnt/tmp331,lowerdir+=/mnt/tmp332,lowerdir+=/mnt/tmp333, lowerdir+=/mnt/tmp334,lowerdir+=/mnt/tmp335,lowerdir+=/mnt/tmp336, lowerdir+=/mnt/tmp337,lowerdir+=/mnt/tmp338,lowerdir+=/mnt/tmp339, lowerdir+=/mnt/tmp340,lowerdir+=/mnt/tmp341,lowerdir+=/mnt/tmp342, lowerdir+=/mnt/tmp343,lowerdir+=/mnt/tmp344,lowerdir+=/mnt/tmp345, lowerdir+=/mnt/tmp346,lowerdir+=/mnt/tmp347,lowerdir+=/mnt/tmp348, lowerdir+=/mnt/tmp349,lowerdir+=/mnt/tmp350,lowerdir+=/mnt/tmp351, lowerdir+=/mnt/tmp352,lowerdir+=/mnt/tmp353,lowerdir+=/mnt/tmp354, lowerdir+=/mnt/tmp355,lowerdir+=/mnt/tmp356,lowerdir+=/mnt/tmp357, lowerdir+=/mnt/tmp358,lowerdir+=/mnt/tmp359,lowerdir+=/mnt/tmp360, lowerdir+=/mnt/tmp361,lowerdir+=/mnt/tmp362,lowerdir+=/mnt/tmp363, lowerdir+=/mnt/tmp364,lowerdir+=/mnt/tmp365,lowerdir+=/mnt/tmp366, lowerdir+=/mnt/tmp367,lowerdir+=/mnt/tmp368,lowerdir+=/mnt/tmp369, lowerdir+=/mnt/tmp370,lowerdir+=/mnt/tmp371,lowerdir+=/mnt/tmp372, lowerdir+=/mnt/tmp373,lowerdir+=/mnt/tmp374,lowerdir+=/mnt/tmp375, lowerdir+=/mnt/tmp376,lowerdir+=/mnt/tmp377,lowerdir+=/mnt/tmp378, lowerdir+=/mnt/tmp379,lowerdir+=/mnt/tmp380,lowerdir+=/mnt/tmp381, lowerdir+=/mnt/tmp382,lowerdir+=/mnt/tmp383,lowerdir+=/mnt/tmp384, lowerdir+=/mnt/tmp385,lowerdir+=/mnt/tmp386,lowerdir+=/mnt/tmp387, lowerdir+=/mnt/tmp388,lowerdir+=/mnt/tmp389,lowerdir+=/mnt/tmp390, lowerdir+=/mnt/tmp391,lowerdir+=/mnt/tmp392,lowerdir+=/mnt/tmp393, lowerdir+=/mnt/tmp394,lowerdir+=/mnt/tmp395,lowerdir+=/mnt/tmp396, lowerdir+=/mnt/tmp397,lowerdir+=/mnt/tmp398,lowerdir+=/mnt/tmp399, lowerdir+=/mnt/tmp400,lowerdir+=/mnt/tmp401,lowerdir+=/mnt/tmp402, lowerdir+=/mnt/tmp403,lowerdir+=/mnt/tmp404,lowerdir+=/mnt/tmp405, lowerdir+=/mnt/tmp406,lowerdir+=/mnt/tmp407,lowerdir+=/mnt/tmp408, lowerdir+=/mnt/tmp409,lowerdir+=/mnt/tmp410,lowerdir+=/mnt/tmp411, lowerdir+=/mnt/tmp412,lowerdir+=/mnt/tmp413,lowerdir+=/mnt/tmp414, lowerdir+=/mnt/tmp415,lowerdir+=/mnt/tmp416,lowerdir+=/mnt/tmp417, lowerdir+=/mnt/tmp418,lowerdir+=/mnt/tmp419,lowerdir+=/mnt/tmp420, lowerdir+=/mnt/tmp421,lowerdir+=/mnt/tmp422,lowerdir+=/mnt/tmp423, lowerdir+=/mnt/tmp424,lowerdir+=/mnt/tmp425,lowerdir+=/mnt/tmp426, lowerdir+=/mnt/tmp427,lowerdir+=/mnt/tmp428,lowerdir+=/mnt/tmp429, lowerdir+=/mnt/tmp430,lowerdir+=/mnt/tmp431,lowerdir+=/mnt/tmp432, lowerdir+=/mnt/tmp433,lowerdir+=/mnt/tmp434,lowerdir+=/mnt/tmp435, lowerdir+=/mnt/tmp436,lowerdir+=/mnt/tmp437,lowerdir+=/mnt/tmp438, lowerdir+=/mnt/tmp439,lowerdir+=/mnt/tmp440,lowerdir+=/mnt/tmp441, lowerdir+=/mnt/tmp442,lowerdir+=/mnt/tmp443,lowerdir+=/mnt/tmp444, lowerdir+=/mnt/tmp445,lowerdir+=/mnt/tmp446,lowerdir+=/mnt/tmp447, lowerdir+=/mnt/tmp448,lowerdir+=/mnt/tmp449,lowerdir+=/mnt/tmp450, lowerdir+=/mnt/tmp451,lowerdir+=/mnt/tmp452,lowerdir+=/mnt/tmp453, lowerdir+=/mnt/tmp454,lowerdir+=/mnt/tmp455,lowerdir+=/mnt/tmp456, lowerdir+=/mnt/tmp457,lowerdir+=/mnt/tmp458,lowerdir+=/mnt/tmp459, lowerdir+=/mnt/tmp460,lowerdir+=/mnt/tmp461,lowerdir+=/mnt/tmp462, lowerdir+=/mnt/tmp463,lowerdir+=/mnt/tmp464,lowerdir+=/mnt/tmp465, lowerdir+=/mnt/tmp466,lowerdir+=/mnt/tmp467,lowerdir+=/mnt/tmp468, lowerdir+=/mnt/tmp469,lowerdir+=/mnt/tmp470,lowerdir+=/mnt/tmp471, lowerdir+=/mnt/tmp472,lowerdir+=/mnt/tmp473,lowerdir+=/mnt/tmp474, lowerdir+=/mnt/tmp475,lowerdir+=/mnt/tmp476,lowerdir+=/mnt/tmp477, lowerdir+=/mnt/tmp478,lowerdir+=/mnt/tmp479,lowerdir+=/mnt/tmp480, lowerdir+=/mnt/tmp481,lowerdir+=/mnt/tmp482,lowerdir+=/mnt/tmp483, lowerdir+=/mnt/tmp484,lowerdir+=/mnt/tmp485,lowerdir+=/mnt/tmp486, lowerdir+=/mnt/tmp487,lowerdir+=/mnt/tmp488,lowerdir+=/mnt/tmp489, lowerdir+=/mnt/tmp490,lowerdir+=/mnt/tmp491,lowerdir+=/mnt/tmp492, lowerdir+=/mnt/tmp493,lowerdir+=/mnt/tmp494,lowerdir+=/mnt/tmp495, lowerdir+=/mnt/tmp496,lowerdir+=/mnt/tmp497,lowerdir+=/mnt/tmp498, lowerdir+=/mnt/tmp499,lowerdir+=/mnt/tmp500,upperdir=/mnt/upper,workdir=/mnt/work uuid=on)
On Tue, Jun 25, 2024 at 12:42:14PM +0200, Christian Brauner wrote: > On Mon, Jun 24, 2024 at 03:40:49PM GMT, Josef Bacik wrote: > > Hello, > > > > Currently if you want to get mount options for a mount and you're using > > statmount(), you still have to open /proc/mounts to parse the mount options. > > statmount() does have the ability to store an arbitrary string however, > > additionally the way we do that is with a seq_file, which is also how we use > > ->show_options for the individual file systems. > > > > Extent statmount() to have a flag for fetching the mount options of a mount. > > This allows users to not have to parse /proc mount for anything related to a > > mount. I've extended the existing statmount() test to validate this feature > > works as expected. As you can tell from the ridiculous amount of silly string > > parsing, this is a huge win for users and climate change as we will no longer > > have to waste several cycles parsing strings anymore. > > > > This is based on my branch that extends listmount/statmount to walk into foreign > > namespaces. Below are links to that posting, that branch, and this branch to > > make it easier to review. > > So I was very hesitant to do it this way because I feel this is pretty > ugly dumping mount options like that but Karel and others have the same > use-case that they want to retrieve it all via statmount() (or another > ID-based system call) so I guess I'll live with this. But note that this > will be a fairly ugly interface at times. For example, mounting overlayfs with > 500 lower layers then what one gets is: > Yeah this isn't awesome, but neither is the string parsing code I have in the selftest to validate this feature. I chose this approach because 1) it's simple and I'm lazy, and 2) I think anything else becomes a lot more complicated and more work than what people actually want. I imagine what would be ideal would be some sort of mount option iterator mechanism. Either we shoe-horn it into statmount() or we add a listmountoptions() syscall, and we then get back a list of mount options individually laid out. This means we now have to keep track of where we are in our mount option traversal, and change all the ->show_options callbacks to handle this new iter thing. We could go this way I suppose, but again this is a lot of work, and honestly I just want to log mount options into some database so I can go looking for people doing weird shit on my giant fleet of machines/containers. Would the iter thing make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, we just want all the options, and we can all strsep/strtok with a comma. Thanks, Josef
On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > We could go this way I suppose, but again this is a lot of work, and honestly I > just want to log mount options into some database so I can go looking for people > doing weird shit on my giant fleet of machines/containers. Would the iter thing > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > we just want all the options, and we can all strsep/strtok with a comma. I think we can live with the monolithic option block. However I'd prefer the separator to be a null character, thus the options could be sent unescaped. That way the iterator will be a lot simpler to implement. Thanks, Miklos
On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote: > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > > > We could go this way I suppose, but again this is a lot of work, and honestly I > > just want to log mount options into some database so I can go looking for people > > doing weird shit on my giant fleet of machines/containers. Would the iter thing > > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > > we just want all the options, and we can all strsep/strtok with a comma. > > I think we can live with the monolithic option block. However I'd > prefer the separator to be a null character, thus the options could be > sent unescaped. That way the iterator will be a lot simpler to > implement. For libmount it means writing a new parser and Karel prefers the "," format so I would like to keep the current format.
On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote: > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote: > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > We could go this way I suppose, but again this is a lot of work, and honestly I > > > just want to log mount options into some database so I can go looking for people > > > doing weird shit on my giant fleet of machines/containers. Would the iter thing > > > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > > > we just want all the options, and we can all strsep/strtok with a comma. > > > > I think we can live with the monolithic option block. However I'd > > prefer the separator to be a null character, thus the options could be > > sent unescaped. That way the iterator will be a lot simpler to > > implement. > > For libmount it means writing a new parser and Karel prefers the "," > format so I would like to keep the current format. Sorry for the misunderstanding. I had a chat with Christian about it when I was out of my office (and phone chats are not ideal for this). I thought Miklos had suggested using a space (" ") as the separator, but after reading the entire email thread, I now understand that Miklos' suggestion is to use \0 (zero) as the options separator. I have no issue with using \0, as it will make things much simpler. Karel
On Tue, Jun 25, 2024 at 03:52:03PM GMT, Karel Zak wrote: > On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote: > > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote: > > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > > We could go this way I suppose, but again this is a lot of work, and honestly I > > > > just want to log mount options into some database so I can go looking for people > > > > doing weird shit on my giant fleet of machines/containers. Would the iter thing > > > > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > > > > we just want all the options, and we can all strsep/strtok with a comma. > > > > > > I think we can live with the monolithic option block. However I'd > > > prefer the separator to be a null character, thus the options could be > > > sent unescaped. That way the iterator will be a lot simpler to > > > implement. > > > > For libmount it means writing a new parser and Karel prefers the "," > > format so I would like to keep the current format. > > Sorry for the misunderstanding. I had a chat with Christian about it > when I was out of my office (and phone chats are not ideal for this). > > I thought Miklos had suggested using a space (" ") as the separator, but > after reading the entire email thread, I now understand that Miklos' > suggestion is to use \0 (zero) as the options separator. > > I have no issue with using \0, as it will make things much simpler. Ah, I misread that myself. Fine by me.
On Tue, Jun 25, 2024 at 03:52:03PM +0200, Karel Zak wrote: > On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote: > > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote: > > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > > We could go this way I suppose, but again this is a lot of work, and honestly I > > > > just want to log mount options into some database so I can go looking for people > > > > doing weird shit on my giant fleet of machines/containers. Would the iter thing > > > > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > > > > we just want all the options, and we can all strsep/strtok with a comma. > > > > > > I think we can live with the monolithic option block. However I'd > > > prefer the separator to be a null character, thus the options could be > > > sent unescaped. That way the iterator will be a lot simpler to > > > implement. > > > > For libmount it means writing a new parser and Karel prefers the "," > > format so I would like to keep the current format. > > Sorry for the misunderstanding. I had a chat with Christian about it > when I was out of my office (and phone chats are not ideal for this). > > I thought Miklos had suggested using a space (" ") as the separator, but > after reading the entire email thread, I now understand that Miklos' > suggestion is to use \0 (zero) as the options separator. > > I have no issue with using \0, as it will make things much simpler. What I mean was "we can all strsep/strtok with a comma" I meant was in userspace. statmount() gives you the giant block, it's up to user space to parse it. I can change the kernel to do this for you, and then add a mnt_opts_len field so you know how big of a block you get. But that means getting the buffer, and going back through it and replacing every ',' with a '\0', because I'm sure as hell not going and changing all of our ->show_options() callbacks to not put in a ','. Is this the direction we want to go? Thanks, Josef
On Tue, Jun 25, 2024 at 10:17:56AM GMT, Josef Bacik wrote: > On Tue, Jun 25, 2024 at 03:52:03PM +0200, Karel Zak wrote: > > On Tue, Jun 25, 2024 at 03:35:17PM GMT, Christian Brauner wrote: > > > On Tue, Jun 25, 2024 at 03:04:40PM GMT, Miklos Szeredi wrote: > > > > On Tue, 25 Jun 2024 at 15:00, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > > > > > We could go this way I suppose, but again this is a lot of work, and honestly I > > > > > just want to log mount options into some database so I can go looking for people > > > > > doing weird shit on my giant fleet of machines/containers. Would the iter thing > > > > > make the overlayfs thing better? Yeah for sure. Do we care? I don't think so, > > > > > we just want all the options, and we can all strsep/strtok with a comma. > > > > > > > > I think we can live with the monolithic option block. However I'd > > > > prefer the separator to be a null character, thus the options could be > > > > sent unescaped. That way the iterator will be a lot simpler to > > > > implement. > > > > > > For libmount it means writing a new parser and Karel prefers the "," > > > format so I would like to keep the current format. > > > > Sorry for the misunderstanding. I had a chat with Christian about it > > when I was out of my office (and phone chats are not ideal for this). > > > > I thought Miklos had suggested using a space (" ") as the separator, but > > after reading the entire email thread, I now understand that Miklos' > > suggestion is to use \0 (zero) as the options separator. > > > > I have no issue with using \0, as it will make things much simpler. > > What I mean was "we can all strsep/strtok with a comma" I meant was in > userspace. statmount() gives you the giant block, it's up to user space to > parse it. > > I can change the kernel to do this for you, and then add a mnt_opts_len field so > you know how big of a block you get. > > But that means getting the buffer, and going back through it and replacing every > ',' with a '\0', because I'm sure as hell not going and changing all of our > ->show_options() callbacks to not put in a ','. > > Is this the direction we want to go? Thanks, Honestly, I don't have a strong opinion on this. Both the ',' and \0 options work for me, and the userspace is already set up for ','. Using \0 is a possibility for creating an ideal kernel interface as it doesn't require escaping, but it may require significant changes to the kernel with minimal advantage for userspace. By the way, starmount() is so flexible that adding support for other formats can be done anytime later with some new STATMOUNT_* mask. Karel
On Mon, 24 Jun 2024 15:40:49 -0400, Josef Bacik wrote: > Currently if you want to get mount options for a mount and you're using > statmount(), you still have to open /proc/mounts to parse the mount options. > statmount() does have the ability to store an arbitrary string however, > additionally the way we do that is with a seq_file, which is also how we use > ->show_options for the individual file systems. > > Extent statmount() to have a flag for fetching the mount options of a mount. > This allows users to not have to parse /proc mount for anything related to a > mount. I've extended the existing statmount() test to validate this feature > works as expected. As you can tell from the ridiculous amount of silly string > parsing, this is a huge win for users and climate change as we will no longer > have to waste several cycles parsing strings anymore. > > [...] * Changed to only call sb->d_op->show_options() so we only show filesystem mount options. * Fixed/tweaked selftests to parse /proc/self/mountinfo directly and look for mount options, skipping over vfs generic superblock mount options. * Since Karel is fine with keeping "," let's keep it. --- Applied to the vfs.mount branch of the vfs/vfs.git tree. Patches in the vfs.mount 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.mount [1/4] fs: rename show_mnt_opts -> show_vfsmnt_opts https://git.kernel.org/vfs/vfs/c/429fc05aefd3 [3/4] fs: export mount options via statmount() https://git.kernel.org/vfs/vfs/c/f363afa8cbe0 [4/4] sefltests: extend the statmount test for mount options https://git.kernel.org/vfs/vfs/c/06bedc037f74
On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote: > But that means getting the buffer, and going back through it and replacing every > ',' with a '\0', because I'm sure as hell not going and changing all of our > ->show_options() callbacks to not put in a ','. > > Is this the direction we want to go? IMO yes. Having a clean interface is much more important than doing slightly less processing on the kernel side (which would likely be done anyway on the user side). Thanks, Miklos
On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote: > > > But that means getting the buffer, and going back through it and replacing every > > ',' with a '\0', because I'm sure as hell not going and changing all of our > > ->show_options() callbacks to not put in a ','. > > > > Is this the direction we want to go? > > IMO yes. Having a clean interface is much more important than doing > slightly less processing on the kernel side (which would likely be > done anyway on the user side). So I went for an extended leave, and this interface was merged in the meantime with much to be desired. The options are presented just the same as in /proc/self/mountinfo (just the standard options left out). And that has all the same problems: - options can't contain commas (this causes much headache for overlayfs which has filenames in its options) - to allow the result to be consumed by fsconfig() for example options need to be unescaped - mnt_opts is confusing, since these are *not* mount options, these are super block options. This patchset was apparently hurried through without much thought and review, and what review I did provide was ignored. So I'm frustrated, but not sure what if anything can be done at this point, since the interface went live in the last release and changing it would probably break things... Thanks, Miklos
On Mon, Nov 11, 2024 at 02:12:16PM +0100, Miklos Szeredi wrote: > On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > But that means getting the buffer, and going back through it and replacing every > > > ',' with a '\0', because I'm sure as hell not going and changing all of our > > > ->show_options() callbacks to not put in a ','. > > > > > > Is this the direction we want to go? > > > > IMO yes. Having a clean interface is much more important than doing > > slightly less processing on the kernel side (which would likely be > > done anyway on the user side). > > So I went for an extended leave, and this interface was merged in the > meantime with much to be desired. > > The options are presented just the same as in /proc/self/mountinfo > (just the standard options left out). And that has all the same > problems: > > - options can't contain commas (this causes much headache for > overlayfs which has filenames in its options) > > - to allow the result to be consumed by fsconfig() for example > options need to be unescaped > > - mnt_opts is confusing, since these are *not* mount options, these > are super block options. > > This patchset was apparently hurried through without much thought and > review, and what review I did provide was ignored. So I'm > frustrated, but not sure what if anything can be done at this point, > since the interface went live in the last release and changing it > would probably break things... I understand your frustation but multiple people agreed that the interface as is is fine and Karel as main consumer agreed as well. So ultimately I didn't see a reason to delay the patchset. None of the issues you raised are really things that make the interface uncomsumable and Karel succeeded to port libmount to the new interfaces with success (minus the mnt_devname we're adding now that he requested) and was happy. If there's genuine behavioral problems that cause substatntial issues for userspace then I would request that you please add a new flag that changes escaping and parsing behavior for statmount().
On Mon, 11 Nov 2024 at 14:29, Christian Brauner <brauner@kernel.org> wrote: > I understand your frustation but multiple people agreed that the > interface as is is fine and Karel as main consumer agreed as well. So > ultimately I didn't see a reason to delay the patchset. This was actually in the first versions that I sent out, but then was removed per your request. Then Josef's crufty version added back. Yeah, for libmount it's fine, but the de-crufted version would've been alright as well. Oh, well... > None of the issues you raised are really things that make the interface > uncomsumable and Karel succeeded to port libmount to the new interfaces > with success (minus the mnt_devname we're adding now that he requested) > and was happy. The problem is with non-libmount users. They won't implement unescaping until they run into trouble. And that will be too late because these cases are rare. > If there's genuine behavioral problems that cause substatntial issues > for userspace then I would request that you please add a new flag that > changes escaping and parsing behavior for statmount(). Need to take a look at what this now does to overlayfs filenames with commas and other special chars in them and see if it can be salvaged. Thanks, Miklos
On Mon, Nov 11, 2024 at 02:12:16PM +0100, Miklos Szeredi wrote: > On Wed, 26 Jun 2024 at 14:23, Miklos Szeredi <miklos@szeredi.hu> wrote: > > > > On Tue, 25 Jun 2024 at 16:18, Josef Bacik <josef@toxicpanda.com> wrote: > > > > > But that means getting the buffer, and going back through it and replacing every > > > ',' with a '\0', because I'm sure as hell not going and changing all of our > > > ->show_options() callbacks to not put in a ','. > > > > > > Is this the direction we want to go? > > > > IMO yes. Having a clean interface is much more important than doing > > slightly less processing on the kernel side (which would likely be > > done anyway on the user side). > > So I went for an extended leave, and this interface was merged in the > meantime with much to be desired. > > The options are presented just the same as in /proc/self/mountinfo > (just the standard options left out). And that has all the same > problems: > > - options can't contain commas (this causes much headache for > overlayfs which has filenames in its options) > > - to allow the result to be consumed by fsconfig() for example > options need to be unescaped > > - mnt_opts is confusing, since these are *not* mount options, these > are super block options. > > This patchset was apparently hurried through without much thought and > review, and what review I did provide was ignored. So I'm > frustrated, but not sure what if anything can be done at this point, > since the interface went live in the last release and changing it > would probably break things... My apologies Miklos, I value your opinion and your feedback. Sending my mind back to when we were discussing this I think it just got lost in the other patchsets I was working on, and then it got merged so it was "ok that's done, next thing." That's my bad, I'll be more careful in the future. Thanks, Josef
On Mon, 11 Nov 2024 at 16:28, Josef Bacik <josef@toxicpanda.com> wrote: > My apologies Miklos, I value your opinion and your feedback. Sending my mind > back to when we were discussing this I think it just got lost in the other > patchsets I was working on, and then it got merged so it was "ok that's done, > next thing." That's my bad, I'll be more careful in the future. Thanks, Thanks, Josef. Sorry about getting a bit emotional, it wasn't totally fair, given that I went offline for two months... As Christian said, we can add a new flag for the un-escaped variant, if needed. I'll make a patch, because I think it would be better if there was a variant that non-libmount users could use without having to bother with unescaping the options. Maybe having two variants isn't such a bad thing, as the current version is still useful for getting a single printable string. Thanks, Miklos
On Mon, Nov 11, 2024 at 05:02:10PM GMT, Miklos Szeredi wrote: > As Christian said, we can add a new flag for the un-escaped variant, if needed. > > I'll make a patch, because I think it would be better if there was a > variant that non-libmount users could use without having to bother > with unescaping the options. Maybe having two variants isn't such a > bad thing, as the current version is still useful for getting a single > printable string. I believe the ideal solution would be to support both variants. The comma-separated variant is not a mistake, in my opinion. It is a very common formatting style that has been used for decades. Karel