Repository: pkolaczk/fclones (language: rust)

Issue:
Warning message confusing when physical file data location not available
I get the following messages running   fclones group yyy :

[2021-10-24 14:33:02.212] fclones:  warn: Failed to fetch extents for file <xxx>: Operation not supported (os error 95)

Maybe it's harmless, my problem is that I don't know what it means.
I am using version:   fclones 0.17.0
system is    xubuntu 20.04  (everything upgraded)
I run fclones on   aboaut 2TiB of data - 200000 files of all sizes 
filesystem is zfs (no mirror or raid but encrypted)
disk is spinning disk 4TiB

Warning occurs on 10 files of the 200000
Any ideas? Could the warning message be somewhat more verbose?
Can I just ignore it?

By the way - I have run a brief speed comparison on that data above - here are my results:
(Intel quad core - 16GB ram - disk cache fully loaded from previous operations)
fclones group  xxx                34 min
rdfind xxx                              81 min
jdupes -S -M -Q -r xxx          90 min
rmlint -T df xxx                     134 min
Pretty impresive!!!




Discussion / hints:
Since you're doing comparisons, if you have a chance to try out https://github.com/jvirkki/dupd on your particular data set, would be interesting to see (dupd scan -p xxx).
This is generally harmless. It means your file system doesn't support getting physical locations of files through the sysctl fiemap API, hence performance is probably much slower than it could be. Anyway glad it is fast despite that optimisation not kicking in.

I'm leaving this open to improve logging.


This is happening for me with ZFS on Ubuntu 20.10 FWIW. Let me know if I can get any more information for you.
I have everything I need. I need to change the message. 
Issue is clear for fclones then.

Might be an openZFS bug ...

Thanks for the quick reaction

Fix patch:
diff --git a/src/device.rs b/src/device.rs
index 721e982..3432b43 100644
--- a/src/device.rs
+++ b/src/device.rs
@@ -51,6 +51,7 @@ pub struct DiskDevice {
     pub index: usize,
     pub name: OsString,
     pub disk_type: DiskType,
+    pub file_system: String,
     pub parallelism: Parallelism,
     seq_thread_pool: Lazy<ThreadPool>,
     rand_thread_pool: Lazy<ThreadPool>,
@@ -61,12 +62,14 @@ impl DiskDevice {
         index: usize,
         name: OsString,
         disk_type: DiskType,
+        file_system: String,
         parallelism: Parallelism,
     ) -> DiskDevice {
         DiskDevice {
             index,
             name,
             disk_type,
+            file_system,
             parallelism,
             seq_thread_pool: Lazy::new(),
             rand_thread_pool: Lazy::new(),
@@ -144,6 +147,7 @@ impl DiskDevices {
             0,
             OsString::from("/"),
             disk_type,
+            String::from("unknown"),
             Parallelism {
                 random: parallelism,
                 sequential: parallelism,
@@ -194,6 +198,7 @@ impl DiskDevices {
         &mut self,
         name: OsString,
         disk_type: DiskType,
+        file_system: String,
         pool_sizes: &HashMap<OsString, Parallelism>,
     ) -> usize {
         if let Some((index, _)) = self.devices.iter().find_position(|d| d.name == name) {
@@ -201,8 +206,13 @@ impl DiskDevices {
         } else {
             let index = self.devices.len();
             let parallelism = Self::get_parallelism(&name, disk_type, pool_sizes);
-            self.devices
-                .push(DiskDevice::new(index, name, disk_type, parallelism));
+            self.devices.push(DiskDevice::new(
+                index,
+                name,
+                disk_type,
+                file_system,
+                parallelism,
+            ));
             index
         }
     }
@@ -238,11 +248,20 @@ impl DiskDevices {
         };
 
         // Default device used when we don't find any real device
-        result.add_device(OsString::from("default"), DiskType::Unknown(-1), pool_sizes);
-
+        result.add_device(
+            OsString::from("default"),
+            DiskType::Unknown(-1),
+            String::from("unknown"),
+            pool_sizes,
+        );
         for d in sys.get_disks() {
             let device_name = Self::physical_device_name(d.get_name());
-            let index = result.add_device(device_name, d.get_type(), pool_sizes);
+            let index = result.add_device(
+                device_name,
+                d.get_type(),
+                String::from_utf8_lossy(d.get_file_system()).to_string(),
+                pool_sizes,
+            );
             result
                 .mount_points
                 .push((Path::from(d.get_mount_point()), index));
diff --git a/src/lib.rs b/src/lib.rs
index 955f43a..633ceea 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -576,8 +576,6 @@ fn update_file_locations(ctx: &AppCtx<'_>, groups: &mut Vec<FileGroup<FileInfo>>
         let progress = ctx.log.progress_bar("Fetching extents", count as u64);
 
         let err_counters = atomic_counter_vec(ctx.devices.len());
-        const MAX_ERR_COUNT_TO_LOG: u32 = 10;
-
         groups
             .par_iter_mut()
             .flat_map(|g| &mut g.files)
@@ -585,20 +583,7 @@ fn update_file_locations(ctx: &AppCtx<'_>, groups: &mut Vec<FileGroup<FileInfo>>
                 let device: &DiskDevice = &ctx.devices[fi.get_device_index()];
                 if device.disk_type != DiskType::SSD {
                     if let Err(e) = fi.fetch_physical_location() {
-                        let counter = &err_counters[device.index];
-                        if counter.load(Ordering::Relaxed) < MAX_ERR_COUNT_TO_LOG {
-                            let err_count = counter.fetch_add(1, Ordering::Relaxed);
-                            ctx.log.warn(format!(
-                                "Failed to fetch extents for file {}: {}",
-                                fi.path, e
-                            ));
-                            if err_count == MAX_ERR_COUNT_TO_LOG {
-                                ctx.log.warn(
-                                    "Too many fetch extents errors. More errors will be ignored. \
-                                 Random access performance might be affected on spinning drives.",
-                                )
-                            }
-                        }
+                        handle_fetch_physical_location_err(ctx, &err_counters, fi, e)
                     }
                 }
             })
@@ -606,6 +591,50 @@ fn update_file_locations(ctx: &AppCtx<'_>, groups: &mut Vec<FileGroup<FileInfo>>
     }
 }
 
+/// Displays a warning message after fiemap ioctl fails and we don't know where the
+/// file data are located.
+/// The `err_counters` array is used to keep track of the number of errors recorded so far for
+/// given device - this array must contain the same number of entries as there are devices.
+/// If there are too many errors, subsequent warnings for the device are suppressed.
+#[cfg(target_os = "linux")]
+fn handle_fetch_physical_location_err(
+    ctx: &AppCtx<'_>,
+    err_counters: &[AtomicU32],
+    file_info: &FileInfo,
+    error: io::Error,
+) {
+    const MAX_ERR_COUNT_TO_LOG: u32 = 10;
+    let device = &ctx.devices[file_info.get_device_index()];
+    let counter = &err_counters[device.index];
+    if error.kind() == io::ErrorKind::Unsupported || error.raw_os_error() == Some(libc::EOPNOTSUPP)
+    {
+        if counter.swap(MAX_ERR_COUNT_TO_LOG, Ordering::Release) < MAX_ERR_COUNT_TO_LOG {
+            ctx.log.warn(format!(
+                "File system {} on device {} doesn't support FIEMAP ioctl API. \
+                This is generally harmless, but random access performance might be decreased \
+                because fclones can't determine physical on-disk location of file data needed \
+                for reading files in the optimal order.",
+                device.file_system,
+                device.name.to_string_lossy()
+            ));
+        }
+    } else if counter.load(Ordering::Acquire) < MAX_ERR_COUNT_TO_LOG {
+        ctx.log.warn(format!(
+            "Failed to fetch file extents mapping for file {}: {}. \
+            This is generally harmless, but it might decrease random access performance.",
+            file_info.path, error
+        ));
+        let err_count = counter.fetch_add(1, Ordering::AcqRel);
+        if err_count == MAX_ERR_COUNT_TO_LOG {
+            ctx.log.warn(format!(
+                "Too many errors trying to fetch file extent mappings on device {}. \
+                Subsequent errors for this device will be ignored.",
+                device.name.to_string_lossy()
+            ))
+        }
+    }
+}
+
 /// Transforms files by piping them to an external program and groups them by their hashes
 fn group_transformed(
     ctx: &AppCtx<'_>,


Test patch: