fix: race condition for device error propagation by madjesc · Pull Request #620 · LinearTapeFileSystem/ltfs

madjesc · 2026-06-22T16:10:51Z

fix: file backend NO_SENSE errortype when it should be backwards
fix: unified simple problems

Changelog

Fixed

Race Condition in Device Error Propagation

Resolved race condition in error propagation mechanism for device operations
Centralized write error handling in iosched.c to prevent inconsistent error states across multiple code paths

Error Type Correction in File Debug Driver

Fixed incorrect error type mapping in filedebug_tc.c where NO_SENSE and WRITE_PERM error types were reversed
Now correctly returns EDEV_WRITE_PERM when force_errortype is set, and EDEV_NO_SENSE otherwise

Write Permission Error Handling

Improved handling of write permission errors by consolidating logic in I/O scheduler layer
Removed redundant MAM (Medium Auxiliary Memory) volume lock updates from multiple locations
Centralized MAM lock status updates to prevent race conditions during error scenarios

Changed

Code Refactoring

Moved _unified_write_index_after_perm function from unified.c to iosched.c as _iosched_write_index_after_perm
Removed duplicate error handling code from unified scheduler
Simplified error return logic in ltfs_fsops_write function

Error Handling Architecture

Consolidated write error handling at the I/O scheduler level
Removed scattered MAM volume lock updates from ltfs_write_index function
Improved error propagation flow from low-level write operations to higher layers

Added

New Error Checking Macro

Added IS_RW_PERM macro in ltfs_error.h for checking read/write permission errors

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have confirmed my fix is effective or that my feature works

amissael95

Thanks for you efforts and working in this change. Leaving a couple of comments. Please ensure the fix is tested and documented in this PR properly.

amissael95 · 2026-07-15T23:48:04Z

+	// TODO: Add logs to this thing
+	if (ret < 0) {
+	  if (IS_WRITE_PERM(-ret)) {
+			mam_lockval vollock = d->matches_name_criteria ? PWE_MAM_IP : PWE_MAM_DP;


Fix indentation please.

amissael95 · 2026-09-23T23:49:09Z

+	}

-	return ret;
+	return ret > (ssize_t)size? (ssize_t)size : ret;


This was also done before this PR change in lines 233 - 334. It looks like it is done to ensure than the return value is always equal to the number of bytes (size variable) wanted to be written, this is in case unified_write returns more bytes than the bytes wanted to write because a "bug":

* @param size Number of bytes to write. *[...] * @return Number of bytes written on success, or a negative value on error. The number of * bytes written on success always equals 'size'; any other nonnegative return value from * this function is a bug. In the future, the scheduler write interface may change to * return 0 on success. */ ssize_t unified_write(struct dentry *d, const char *buf, size_t size, off_t offset,

Could you confirm it? Do you have more context of how that bug can be generated/triggered?

fix: NO_SENSE errortype when it should be backwards fix: unified simple problems

syaoraang

Overall, they seem pretty good changes, and a good way to overcome the dependency hell for some cases, but I think it can be improved and some stuffs need to be addressed.

syaoraang · 2026-09-25T19:14:55Z

 */

 #include "ltfs.h"
+#include "ltfs_error.h"


Not necessary header, please remove it.

syaoraang · 2026-09-25T19:15:48Z

-	if (ret < 0)
-		return ret;
-	else
-		return 0;
+	return ret < 0? ret : 0;


Works, but it's not necessary to change it since it's already working and this does not changes the behavior.

syaoraang · 2026-09-25T19:18:21Z

-				return -EDEV_NO_SENSE;
+  			return -EDEV_WRITE_PERM;
 			else
-				return -EDEV_WRITE_PERM;
+  			return -EDEV_NO_SENSE;


Nice catch!
But please fix indentation

syaoraang · 2026-09-25T19:20:44Z


 #include "fs.h"
 #include "ltfs.h"
+#include "ltfs_error.h"


Not necessary headder add.

syaoraang · 2026-09-25T19:55:36Z

 		/* Position mismatch, diff not equal zero */
 		ltfsmsg(LTFS_INFO, 17293E, (unsigned long long)physical_selfptr.block, (unsigned long long)current_position.block);
-		return -1;
+		return -LTFS_INDEX_INVALID;


This return value doesn't match with LTFS_INDEX_INVALID definition, besides the comment for this part specifies to return -1, if we change this return value, we should update the comment

syaoraang · 2026-09-25T20:17:06Z

-		if (volstat == PWE_MAM_DP && partition == ltfs_ip_id(vol))
-			new_volstat = PWE_MAM_BOTH;
-		else if (volstat == PWE_MAM_IP && partition == ltfs_dp_id(vol))
-			new_volstat = PWE_MAM_BOTH;
-		else if (volstat == UNLOCKED_MAM && partition == ltfs_ip_id(vol))
-			new_volstat = PWE_MAM_IP;
-		else if (volstat == UNLOCKED_MAM && partition == ltfs_dp_id(vol))
-			new_volstat = PWE_MAM_DP;


I see that the MAM status gotten from volstats is managed in iosched_write(), but this function (ltfs_write_index()) is also called from other functions such as fuse's mount and unmount and some others.
So, we're loosing this status' management and the possible lock.
Is this expected?

syaoraang · 2026-09-25T20:34:42Z

 **
 *************************************************************************************
 */
+#include "ltfs_error.h"


Not needed header

syaoraang · 2026-09-25T20:42:33Z

 					/* Index partition writer: failed to write data to the tape (%d) */
 					ltfsmsg(LTFS_WARN, 13013W, (int)ret);
-					if (IS_WRITE_PERM(-ret)) {
-						ret = tape_set_cart_volume_lock_status(priv->vol, PWE_MAM_IP);


Since this is under the unified and the lock is now in the iosched, we'll lose those guards.
Is this intended?

syaoraang · 2026-09-25T20:43:08Z

 					if (ret < 0) {
 						/* Data partition writer: failed to write data to the tape (%d) */
 						ltfsmsg(LTFS_WARN, 13014W, (int)ret);
-						(void)_unified_write_index_after_perm(ret, priv);


Since this is under the unified and the catch of the perm is now in the iosched, we'll lose that behavior.
Is this intended?
This applies for next 3 changes

Piloalucard · 2026-10-01T18:15:54Z

+	uint64_t last_index_pos = UINT64_MAX;
+	unsigned long blocksize;
+
+	if (!IS_WRITE_PERM(-write_ret)) {


This check is unnecessary if this is going to be used like below, you do the check IS_WRITE_PERM and inside you call this function

madjesc requested review from XV02, amissael95, syaoraang and vandelvan June 22, 2026 16:10

madjesc force-pushed the fix/unified-race-condition branch from befb20a to c0aeabf Compare June 22, 2026 16:47

vandelvan changed the base branch from main to release/v2.4.9.0 July 3, 2026 19:06

amissael95 requested changes Jul 15, 2026

View reviewed changes

madjesc force-pushed the fix/unified-race-condition branch from c0aeabf to 6a00127 Compare July 16, 2026 19:03

amissael95 reviewed Jul 16, 2026

View reviewed changes

Comment thread src/libltfs/ltfs.c

Comment thread src/iosched/unified.c Outdated

madjesc force-pushed the fix/unified-race-condition branch from 6a00127 to 0f0a343 Compare July 22, 2026 17:46

Piloalucard reviewed Jul 22, 2026

View reviewed changes

Comment thread src/iosched/unified.c Outdated

Piloalucard reviewed Jul 22, 2026

View reviewed changes

Comment thread src/libltfs/iosched.c

madjesc force-pushed the fix/unified-race-condition branch from c5d658a to decdff8 Compare September 23, 2026 21:35

amissael95 reviewed Sep 23, 2026

View reviewed changes

mcardenas and others added 2 commits September 25, 2026 04:46

fix: race condition for device error propagation

6a5b8bf

fix: NO_SENSE errortype when it should be backwards fix: unified simple problems

remove unnecesary

57515dd

madjesc force-pushed the fix/unified-race-condition branch from decdff8 to 57515dd Compare September 25, 2026 10:47

syaoraang changed the base branch from release/v2.4.9.0 to release/v2.4.9.1 September 25, 2026 18:59

syaoraang requested changes Sep 25, 2026

View reviewed changes

Piloalucard reviewed Oct 1, 2026

View reviewed changes

Conversation

madjesc commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Fixed

Race Condition in Device Error Propagation

Error Type Correction in File Debug Driver

Write Permission Error Handling

Changed

Code Refactoring

Error Handling Architecture

Added

New Error Checking Macro

Checklist:

Uh oh!

amissael95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syaoraang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

madjesc commented Jun 22, 2026 •

edited

Loading