Skip to content
This repository was archived by the owner on Feb 27, 2023. It is now read-only.

Conversation

@procount
Copy link
Contributor

Now that NOOBS can support installation to USB MSDs, it opens the door to some very large capacity devices. A recent user of PINN reported an inability to install 2 or 3 OSes to a 2TB drive. The same errors occurred in NOOBS consisting of the display of negative sizes and errors creating partitions.

A lot of the variables used to calculate disk offset, sizes and sector numbers are just integers and are too small for these large capacity drives. So I have converted all of them to qint64 and used the .toULongLong() function where appropriate as a proposal.

I haven't got a 2TB disk to test it on myself, but the user reported that he was able to install the OSes with the changes without errors. Therefore you might like to review all of the changes before implementing, but nevertheless support for USB drives >1TB should probably be included now.

@maxnet
Copy link
Collaborator

maxnet commented Jun 22, 2017

A lot of the variables used to calculate disk offset, sizes and sector numbers are just integers

The MBR partition scheme also uses integers.
Bug probably just is that unsigned ones should be used.

@procount
Copy link
Contributor Author

You could be right. I didn't analyse it in detail but in some cases I also found 'int xxxx = yyy.toULongLong'.
I took a very broad brush approach ( and forgot initdrivethread.cpp ) but I needed a quick fix and it seems to work. However, a more focused expert solution is probably required before other users also report this issue.

@procount
Copy link
Contributor Author

I decided to look in a bit more detail.

NOOBS normally measures partition sizes in MiBs, so 32 bits is quite sufficient (for now anyway :D ) But for disk partitioning, this size is converted to the number of sectors (each of 512 bytes). So the maximum number of sectors that can be counted by a 32 bit signed int equates to about 1TB. Converting to unsigned can extend this only to 2TB.

I think 3 & 4TB drives are becoming more affordable these days and users may start to use these bigger disks as storage for all their video files, considering the proliferation of media centre software for the Pi that there is these days.

@maxnet
Copy link
Collaborator

maxnet commented Jun 23, 2017

Converting to unsigned can extend this only to 2TB.

Correct.
That happens to be the maximum a MBR partition table can address.

Like I already mentioned they use ints as well:

https://en.wikipedia.org/wiki/Master_boot_record#Partition_table_entries

+8hex		4 bytes	LBA of first absolute sector in the partition

Would need to implement support for GPT partition tables if you want to support anything >2TB

@procount
Copy link
Contributor Author

Thanks for the pointers and useful info @maxnet. I learn something new everyday!
OK, let's change to uints then.

@procount
Copy link
Contributor Author

void ProgressSlideshowDialog::setMaximum(qint64 bytes)
{
    _maxSectors = bytes/512;
    ui->progressBar->setMaximum(_maxSectors);
}

progressBar->setMaximum(int)

Should we change the scale of this progressbar to 1KB blocks instead of number of sectors, so that it can cope with >1TB ? Or just clamp it at 1TB? Or not worry? or something else?
Suggestions?

@procount
Copy link
Contributor Author

procount commented Jun 23, 2017

Well, I assumed that writing 1TB of OSes is unlikely, so I just clamped it at 1TB just in case. still got no 2TB to test it on, but I'll see if I can get it verified by another means.
It probably needs some more work around the ioaccounting of the progress dialog too. The underlying written sectors may still need to be 64 bit even if the delta is <32 bit

return 0;
numsectors = stats.at(6).toUInt(); /* write sectors */

if (numsectors > 2147483647) //Maybe use MAX_INT from limits.h?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use MAX_INT (for readability)?

@XECDesign
Copy link
Contributor

Is this tested and good to go?

@procount
Copy link
Contributor Author

procount commented Nov 2, 2017

I'll check if I did anymore changes to it in PINN, as it was a while ago...
(You have been busy tidying up today!)

@procount
Copy link
Contributor Author

procount commented Nov 3, 2017

As far as I can tell, it still looks the same in PINN. so it's tested as far as I can without a 2TB drive.
You can use MAX_INT if you prefer.

@XECDesign
Copy link
Contributor

I thought this one was already merged. Before I do, are there any changes you'd like to make, @procount ?

@procount
Copy link
Contributor Author

procount commented Oct 4, 2018

Apparently it has not been merged into NOOBS yet.
AFAICT, I didn't do any other changes to it in PINN after these PR changes

procount and others added 5 commits October 8, 2018 11:37
Conflicts:
	recovery/config.h
	recovery/mainwindow.h
	recovery/progressslideshowdialog.cpp
	recovery/translation_ae.ts
	recovery/translation_ast.ts
	recovery/translation_ca.ts
	recovery/translation_de.ts
	recovery/translation_en.ts
	recovery/translation_es.ts
	recovery/translation_eu.ts
	recovery/translation_fi.ts
	recovery/translation_fr.ts
	recovery/translation_hu.ts
	recovery/translation_it.ts
	recovery/translation_ja.ts
	recovery/translation_ko.ts
	recovery/translation_nl.ts
	recovery/translation_no.ts
	recovery/translation_pl.ts
	recovery/translation_pt.ts
	recovery/translation_ru.ts
	recovery/translation_sv.ts
	recovery/translation_tr.ts
	recovery/translation_zh_TW.ts

fixing partition creation

Conflicts:
	recovery/config.h
	recovery/multiimagewritethread.cpp
@XECDesign XECDesign merged commit 21cf945 into raspberrypi:master Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants