diff --git a/fs_mgr/libfiemap_writer/fiemap_writer.cpp b/fs_mgr/libfiemap_writer/fiemap_writer.cpp index 097e07f02..4a0d4b58d 100644 --- a/fs_mgr/libfiemap_writer/fiemap_writer.cpp +++ b/fs_mgr/libfiemap_writer/fiemap_writer.cpp @@ -165,23 +165,13 @@ static bool FileToBlockDevicePath(const std::string& file_path, std::string* bde return true; } -static bool GetBlockDeviceParams(int bdev_fd, const std::string& bdev_path, uint64_t* blocksz, - uint64_t* bdev_size) { - // TODO: For some reason, the block device ioctl require the argument to be initialized - // to zero even if its the out parameter for the given ioctl cmd. - uint64_t blksz = 0; - if (ioctl(bdev_fd, BLKBSZGET, &blksz)) { - PLOG(ERROR) << "Failed to get block size for: " << bdev_path; - return false; - } - +static bool GetBlockDeviceSize(int bdev_fd, const std::string& bdev_path, uint64_t* bdev_size) { uint64_t size_in_bytes = 0; if (ioctl(bdev_fd, BLKGETSIZE64, &size_in_bytes)) { PLOG(ERROR) << "Failed to get total size for: " << bdev_path; return false; } - *blocksz = blksz; *bdev_size = size_in_bytes; return true; @@ -197,22 +187,20 @@ static uint64_t GetFileSize(const std::string& file_path) { return sb.st_size; } -static bool PerformFileChecks(const std::string& file_path, uint64_t file_size, uint64_t blocksz, +static bool PerformFileChecks(const std::string& file_path, uint64_t file_size, uint64_t* blocksz, uint32_t* fs_type) { - // Check if the size aligned to the block size of the block device. - // We need this to be true in order to be able to write the file using FIEMAP. - if (file_size % blocksz) { - LOG(ERROR) << "File size " << file_size << " is not aligned to block size " << blocksz - << " for file " << file_path; - return false; - } - struct statfs64 sfs; if (statfs64(file_path.c_str(), &sfs)) { PLOG(ERROR) << "Failed to read file system status at: " << file_path; return false; } + if (file_size % sfs.f_bsize) { + LOG(ERROR) << "File size " << file_size << " is not aligned to optimal block size " + << sfs.f_bsize << " for file " << file_path; + return false; + } + // Check if the filesystem is of supported types. // Only ext4 and f2fs are tested and supported. if ((sfs.f_type != EXT4_SUPER_MAGIC) && (sfs.f_type != F2FS_SUPER_MAGIC)) { @@ -226,6 +214,7 @@ static bool PerformFileChecks(const std::string& file_path, uint64_t file_size, return false; } + *blocksz = sfs.f_bsize; *fs_type = sfs.f_type; return true; } @@ -303,7 +292,7 @@ static bool PinFile(int file_fd, const std::string& file_path, uint32_t fs_type) uint32_t pin_status = 1; int error = ioctl(file_fd, F2FS_IOC_SET_PIN_FILE, &pin_status); - if (error) { + if (error < 0) { if ((errno == ENOTTY) || (errno == ENOTSUP)) { PLOG(ERROR) << "Failed to pin file, not supported by kernel: " << file_path; } else { @@ -316,7 +305,7 @@ static bool PinFile(int file_fd, const std::string& file_path, uint32_t fs_type) } #if 0 -static bool PinFileStatus(int file_fd, const std::string& file_path, uint32_t fs_type) { +static bool IsFilePinned(int file_fd, const std::string& file_path, uint32_t fs_type) { if (fs_type == EXT4_SUPER_MAGIC) { // No pinning necessary for ext4. The blocks, once allocated, are expected // to be fixed. @@ -339,9 +328,10 @@ static bool PinFileStatus(int file_fd, const std::string& file_path, uint32_t fs #define F2FS_IOC_GET_PIN_FILE _IOR(F2FS_IOCTL_MAGIC, 14, __u32) #endif - uint32_t pin_status; - int error = ioctl(file_fd, F2FS_IOC_GET_PIN_FILE, &pin_status); - if (error) { + // F2FS_IOC_GET_PIN_FILE returns the number of blocks moved. + uint32_t moved_blocks_nr; + int error = ioctl(file_fd, F2FS_IOC_GET_PIN_FILE, &moved_blocks_nr); + if (error < 0) { if ((errno == ENOTTY) || (errno == ENOTSUP)) { PLOG(ERROR) << "Failed to get file pin status, not supported by kernel: " << file_path; } else { @@ -350,7 +340,10 @@ static bool PinFileStatus(int file_fd, const std::string& file_path, uint32_t fs return false; } - return !!pin_status; + if (moved_blocks_nr) { + LOG(ERROR) << moved_blocks_nr << " blocks moved in file " << file_path; + } + return moved_blocks_nr == 0; } #endif @@ -447,7 +440,7 @@ FiemapUniquePtr FiemapWriter::Open(const std::string& file_path, uint64_t file_s std::string bdev_path; if (!FileToBlockDevicePath(abs_path, &bdev_path)) { LOG(ERROR) << "Failed to get block dev path for file: " << file_path; - cleanup(file_path, create); + cleanup(abs_path, create); return nullptr; } @@ -459,9 +452,9 @@ FiemapUniquePtr FiemapWriter::Open(const std::string& file_path, uint64_t file_s return nullptr; } - uint64_t blocksz, bdevsz; - if (!GetBlockDeviceParams(bdev_fd, bdev_path, &blocksz, &bdevsz)) { - LOG(ERROR) << "Failed to get block device params for: " << bdev_path; + uint64_t bdevsz; + if (!GetBlockDeviceSize(bdev_fd, bdev_path, &bdevsz)) { + LOG(ERROR) << "Failed to get block device size for : " << bdev_path; cleanup(file_path, create); return nullptr; } @@ -474,23 +467,24 @@ FiemapUniquePtr FiemapWriter::Open(const std::string& file_path, uint64_t file_s } } + uint64_t blocksz; uint32_t fs_type; - if (!PerformFileChecks(abs_path, file_size, blocksz, &fs_type)) { + if (!PerformFileChecks(abs_path, file_size, &blocksz, &fs_type)) { LOG(ERROR) << "Failed to validate file or file system for file:" << abs_path; - cleanup(file_path, create); + cleanup(abs_path, create); return nullptr; } if (create) { if (!AllocateFile(file_fd, abs_path, blocksz, file_size)) { - unlink(abs_path.c_str()); + cleanup(abs_path, create); return nullptr; } } // f2fs may move the file blocks around. if (!PinFile(file_fd, file_path, fs_type)) { - cleanup(file_path, create); + cleanup(abs_path, create); LOG(ERROR) << "Failed to pin the file in storage"; return nullptr; } @@ -499,7 +493,7 @@ FiemapUniquePtr FiemapWriter::Open(const std::string& file_path, uint64_t file_s FiemapUniquePtr fmap(new FiemapWriter()); if (!ReadFiemap(file_fd, abs_path, &fmap->extents_)) { LOG(ERROR) << "Failed to read fiemap of file: " << abs_path; - cleanup(file_path, create); + cleanup(abs_path, create); return nullptr; } @@ -543,9 +537,10 @@ bool FiemapWriter::Write(off64_t off, uint8_t* buffer, uint64_t size) { << " for block size " << block_size_; return false; } + #if 0 // TODO(b/122138114): check why this fails. - if (!PinFileStatus(file_fd_, file_path_, fs_type_)) { + if (!IsFilePinned(file_fd_, file_path_, fs_type_)) { LOG(ERROR) << "Failed write: file " << file_path_ << " is not pinned"; return false; } diff --git a/fs_mgr/libfiemap_writer/fiemap_writer_test.cpp b/fs_mgr/libfiemap_writer/fiemap_writer_test.cpp index 6653adabf..6dff0e8b4 100644 --- a/fs_mgr/libfiemap_writer/fiemap_writer_test.cpp +++ b/fs_mgr/libfiemap_writer/fiemap_writer_test.cpp @@ -40,11 +40,22 @@ using namespace android::fiemap_writer; using unique_fd = android::base::unique_fd; using LoopDevice = android::dm::LoopDevice; -std::string testfile = ""; std::string testbdev = ""; uint64_t testfile_size = 536870912; // default of 512MiB -TEST(FiemapWriter, CreateImpossiblyLargeFile) { +class FiemapWriterTest : public ::testing::Test { + protected: + void SetUp() override { + const ::testing::TestInfo* tinfo = ::testing::UnitTest::GetInstance()->current_test_info(); + std::string exec_dir = ::android::base::GetExecutableDirectory(); + testfile = ::android::base::StringPrintf("%s/testdata/%s", exec_dir.c_str(), tinfo->name()); + } + + // name of the file we use for testing + std::string testfile; +}; + +TEST_F(FiemapWriterTest, CreateImpossiblyLargeFile) { // Try creating a file of size ~100TB but aligned to // 512 byte to make sure block alignment tests don't // fail. @@ -54,7 +65,7 @@ TEST(FiemapWriter, CreateImpossiblyLargeFile) { EXPECT_EQ(errno, ENOENT); } -TEST(FiemapWriter, CreateUnalignedFile) { +TEST_F(FiemapWriterTest, CreateUnalignedFile) { // Try creating a file of size 4097 bytes which is guaranteed // to be unaligned to all known block sizes. The creation must // fail. @@ -64,7 +75,7 @@ TEST(FiemapWriter, CreateUnalignedFile) { EXPECT_EQ(errno, ENOENT); } -TEST(FiemapWriter, CheckFilePath) { +TEST_F(FiemapWriterTest, CheckFilePath) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, 4096); ASSERT_NE(fptr, nullptr); EXPECT_EQ(fptr->size(), 4096); @@ -72,20 +83,20 @@ TEST(FiemapWriter, CheckFilePath) { EXPECT_EQ(access(testfile.c_str(), F_OK), 0); } -TEST(FiemapWriter, CheckBlockDevicePath) { +TEST_F(FiemapWriterTest, CheckBlockDevicePath) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, 4096); EXPECT_EQ(fptr->size(), 4096); EXPECT_EQ(fptr->bdev_path(), testbdev); } -TEST(FiemapWriter, CheckFileCreated) { +TEST_F(FiemapWriterTest, CheckFileCreated) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, 32768); ASSERT_NE(fptr, nullptr); unique_fd fd(open(testfile.c_str(), O_RDONLY)); EXPECT_GT(fd, -1); } -TEST(FiemapWriter, CheckFileSizeActual) { +TEST_F(FiemapWriterTest, CheckFileSizeActual) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, testfile_size); ASSERT_NE(fptr, nullptr); @@ -94,13 +105,13 @@ TEST(FiemapWriter, CheckFileSizeActual) { EXPECT_EQ(sb.st_size, testfile_size); } -TEST(FiemapWriter, CheckFileExtents) { +TEST_F(FiemapWriterTest, CheckFileExtents) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, testfile_size); ASSERT_NE(fptr, nullptr); EXPECT_GT(fptr->extents().size(), 0); } -TEST(FiemapWriter, CheckWriteError) { +TEST_F(FiemapWriterTest, CheckWriteError) { FiemapUniquePtr fptr = FiemapWriter::Open(testfile, testfile_size); ASSERT_NE(fptr, nullptr); @@ -339,18 +350,17 @@ TEST_F(VerifyBlockWritesF2fs, CheckWrites) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); - if (argc <= 2) { + if (argc <= 1) { cerr << "Filepath with its bdev path must be provided as follows:" << endl; - cerr << " $ fiemap_writer_test 3) { - testfile_size = strtoull(argv[3], NULL, 0); + testbdev = argv[1]; + if (argc > 2) { + testfile_size = strtoull(argv[2], NULL, 0); if (testfile_size == ULLONG_MAX) { testfile_size = 512 * 1024 * 1024; }