fix: fix get sector size on mips64

[eatradish]

This PR will fix the get_sector_size function returning ENOTTY on the MIPS64 architecture.

[xry111]

See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/ioctl.c?h=v6.14-rc1#n588. I'm still unsure why this only blows up on MIPS64 (FWIW our system is little-endian), I guess it's related to MIPS64 having things like

If either GPR rt or GPR rs does not contain sign-extended 32-bit values (bits 63..31 equal), then the result of the operation is UNPREDICTABLE.

a limitation that most 64-bit architectures don't have.

[cecton]

Hello and welcome 👋

Wow this sounds like a very specific issue! Would it be possible to call the ioctl with signed 32 bits but then still return a u64? Mostly to avoid API breaking change but also I think it's using u64 internally everywhere in gptman. Would that be convenient for your use?

If not we could also use cfg() to limit this change to mips64 only.

Let me know your thoughts on this I'm not really knowledgeable for this.

@bgilbert I would really love your input on this one if you have time ofc

[xry111]

Hello and welcome 👋

Wow this sounds like a very specific issue! Would it be possible to call the ioctl with signed 32 bits but then still return a u64? Mostly to avoid API breaking change but also I think it's using u64 internally everywhere in gptman. Would that be convenient for your use?

Yes, for our use we actually need to cast it to u64:

https://github.com/AOSC-Dev/deploykit-backend/pull/16/files#diff-eff58eaa8ed831ed16d86ac37b8715c5ccd2c58712aeaae3187b5b6a8aaee81dR265

and I'd prefer this way. Technically this is a potential problem for all architectures, so making the fix MIPS64-specific seems not a good idea.

The problem is what we should do if we get a negative value. If the kernel is not buggy that should never happen, but in Rust we have to handle the case. Is .try_into().unwrap() acceptable?

[cecton]

I was thinking about .unwrap_or(512) so it's guaranteed to not cause a panic and it's a reasonable value in most cases. What do you think?

Don't bother with the checks I will fix it myself I have to change the workflow.

[cecton]

If you update your branch with origin/main you should have less issues with the checks on GitHub.

[eatradish]

@cecton @xry111 Like this? d4b21fb

[xry111]

LGTM, maybe add a comment to explain 512 should be only used as a fall back in case the kernel goes crazy.

[cecton]

LGTM, maybe add a comment to explain 512 should be only used as a fall back in case the kernel goes crazy.

Good point, something like:

Under normal conditions, get_sector_size retrieves the logical sector size using the BLKSSZGET ioctl command. If the system returns an error, the function handles it appropriately.

However, in the rare event that the system reports success (0) but writes a negative value, the function will default to 512 bytes to ensure stability. This is a safeguard against unexpected system behavior.

You both agree at 100% with this behavior? It works perfectly for your use case?

[cecton]

oh sorry I meant for the documentation of the function so the user can see it from the doc.

[eatradish]

LGTM, maybe add a comment to explain 512 should be only used as a fall back in case the kernel goes crazy.

Good point, something like:

Under normal conditions, get_sector_size retrieves the logical sector size using the BLKSSZGET ioctl command. If the system returns an error, the function handles it appropriately.
However, in the rare event that the system reports success (0) but writes a negative value, the function will default to 512 bytes to ensure stability. This is a safeguard against unexpected system behavior.

You both agree at 100% with this behavior? It works perfectly for your use case?

I totally agree that this is the way to handle this issue, for our use case we are also using try_into, which shouldn't cause a problem

[eatradish]

oh sorry I meant for the documentation of the function so the user can see it from the doc.

Done