On 5/7/25 20:31, Eric Sandeen wrote: > On 5/7/25 6:26 AM, Chao Yu wrote: >> On 4/20/25 23:25, Eric Sandeen wrote: >>> From: Hongbo Li <lihongbo22@xxxxxxxxxx> >>> >>> In handle_mount_opt, we use fs_parameter to parse each option. >>> However we're still using the old API to get the options string. >>> Using fsparams parse_options allows us to remove many of the Opt_ >>> enums, so remove them. >>> >>> The checkpoint disable cap (or percent) involves rather complex >>> parsing; we retain the old match_table mechanism for this, which >>> handles it well. >>> >>> There are some changes about parsing options: >>> 1. For `active_logs`, `inline_xattr_size` and `fault_injection`, >>> we use s32 type according the internal structure to record the >>> option's value. >> >> We'd better to use u32 type for these options, as they should never >> be negative. >> >> Can you please update based on below patch? >> >> https://lore.kernel.org/linux-f2fs-devel/20250507112425.939246-1-chao@xxxxxxxxxx > > Hi Chao - I agree that that patch makes sense, but maybe there is a timing > issue now? At the moment, there is a mix of signed and unsigned handling > for these options. I agree that the conversion series probably should have > left the parsing type as unsigned, but it was a mix internally, so it was > difficult to know for sure. > > For your patch above, if it is to stand alone or be merged first, it > should probably also change the current parsing to match_uint. (this would > also make it backportable to -stable kernels, if you want to). > > Otherwise, I would suggest that if it is merged after the mount API series, > then your patch to clean up internal types could fix the (new mount API) > parsing from %s to %u at the same time? > > Happy to do it either way but your patch should probably be internally > consistent, changing the parsing types at the same time. > > (I suppose we could incorporate your patch into the mount API series too, > though it'd be a little strange to have a minor bugfix like this buried > in the series.) Eric, Yeah, it's a little bit strange to include that patch into mount API series, now, I realize that rebasing huge change to a minor fix is not necessary, anyway, let me focus on reviewing, and pickup that patch after mount API series be merged. :P Thanks, > > Thanks, > -Eric >
