Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix detect drive type #85

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

JacksonYao287
Copy link
Contributor

in the current code, we use the filename (not the Absolute Path) of the device to get the major and minor number.
while if a physical device is a lvm(logic volume), it will fail to get the major and minor number.

NAME              MAJ:MIN RM  SIZE RO TYPE MOUNTPOINTS
vda               252:0    0  100G  0 disk
├─vda1            252:1    0    1M  0 part
├─vda2            252:2    0  488M  0 part /boot
└─vda3            252:3    0 99.5G  0 part
  └─atomicos-root 253:0    0 99.5G  0 lvm  /var/lib/containers/storage/overlay
                                           /sysroot/ostree/deploy/ubuntu-atomic-host/var/tmp
                                           /var/tmp
                                           /var
                                           /usr
                                           /
                                           /sysroot
vdb               252:16   0   64M  0 disk

# ls -rlt /tmp
lrwxrwxrwx 2 root root 11 Jun 18  2023 /tmp -> sysroot/tmp

#df -h /tmp
Filesystem                 Size  Used Avail Use% Mounted on
/dev/mapper/atomicos-root  100G   56G   44G  57% /sysroot

this is the info got from lsblk in virtual machine, which is my local debug machine. we can see atomicos-root is a lvm device. and /tmp resides in /dev/mapper/atomicos-root

when we run homestore test and create file under /tmp as a device, we can get the following debug info

(gdb) s
iomgr::is_rotational_device (device="atomicos-root") at /var/home/ubuntu/.conan/data/iomgr/11.3.2-35/oss/master/build/8f6113204e39cf5069af1f6032b873fa6df4d478/src/lib/interfaces/drive_interface.cpp:86
86      static bool is_rotational_device(const std::string& device) {
(gdb) n
87          int is_rotational = 0;
(gdb) n
88          const auto maj_min{get_major_minor(device)};
(gdb) p get_major_minor(device)
[06/03/24 01:04:52-07:00] [error] [test_journal_vdev] [762257] [drive_interface.cpp:80:get_major_minor] Unable to stat the path atomicos-root, ignoring to get major/minor, ret:-1
$21 = ""

so we can see if we use the only file name to get the major/minor of a device, we will get an error.

(gdb) p device
$24 = "/dev/mapper/atomicos-root"
(gdb) p get_major_minor(device)
$25 = "253:0"

we can see if we use the Absolute Path to get the major/minor, it works.

this is what this PR do

@@ -200,7 +200,7 @@ static std::string get_raid_hdd_vendor_model() {

drive_type DriveInterface::detect_drive_type(const std::string& dev_name) {
if (std::filesystem::is_regular_file(std::filesystem::status(dev_name))) {
auto device = std::filesystem::path(get_mounted_device(dev_name)).filename();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we used std::filesystem::path() for a reason right? Will it not break that and cause regression? I believe this was done so that if one passes soft/hard link path to the device, it could pick the softlink and treat the size of the device as link size, instead of actual device?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thread 1 "test_journal_vd" hit Breakpoint 1, iomgr::DriveInterface::detect_drive_type (dev_name="/sysroot/tmp/test_journal_vdev_1") 

(gdb) p get_mounted_device(dev_name)
$1 = "/dev/mapper/atomicos-root"

(gdb) p dev_name.assign("/var/home/ubuntu/ttt")
$3 = "/var/home/ubuntu/ttt"
(gdb) p dev_name
$4 = "/var/home/ubuntu/ttt"
(gdb) p get_mounted_device(dev_name)
$5 = "/dev/mapper/atomicos-root"


storage-workstation-gtxgj:~# ln -s /sysroot/tmp/test_journal_vdev_1 ./ttt
storage-workstation-gtxgj:~# ls
2.cpp  go  ttt
storage-workstation-gtxgj:~# pwd
/var/home/ubuntu
storage-workstation-gtxgj:~# ls -rlt .
total 4
drwxrwx--- 3 ubuntu ubuntu  17 Apr  8 00:12 go
-rw-rw---- 1 ubuntu ubuntu 632 Jun  2 23:20 2.cpp
lrwxrwxrwx 1 ubuntu ubuntu  32 Jun  3 09:46 ttt -> /sysroot/tmp/test_journal_vdev_1

I test this using a soft link, and it works as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hkadayam can you recall for what reason we use file name instead of the absolute path to get the the major and minor number of a device

@yamingk
Copy link
Contributor

yamingk commented Jun 3, 2024

Assume that this PR has been run with HomeObj and HomeStore regression test, does make sense to also verify the PR on read machine with real nvme and hard drives? As mentioned, it is there for some reason (may or may not be valid for some reason), but better to verify with real drives also.

Copy link
Contributor

@shosseinimotlagh shosseinimotlagh left a comment

Choose a reason for hiding this comment

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

LG

@JacksonYao287
Copy link
Contributor Author

I merge this PR for now, we can revisit here is any error happens

@JacksonYao287 JacksonYao287 merged commit fe7208b into eBay:master Jun 7, 2024
12 checks passed
@JacksonYao287 JacksonYao287 deleted the fix-detect_drive_type branch June 7, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants