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

bugfix: cannot mount LTFS tapes under Rocky Linux with lin_tape driver #399

Merged

Conversation

richard42
Copy link
Contributor

Summary of changes

This pull request includes one bugfix.

Description

This is a very simple fix. The bug is pretty obvious. It causes all LTFS tape mounts to fail on our new Rocky Linux 8 machine using the lin_tape driver, with a log output like this:

c134 LTFS17089I Distribution: Rocky Linux release 8.7 (Green Obsidian).
c134 LTFS17089I Distribution: Rocky Linux release 8.7 (Green Obsidian).
c134 LTFS17089I Distribution: Rocky Linux release 8.7 (Green Obsidian).
c134 LTFS14064I Sync type is "unmount".
c134 LTFS17085I Plugin: Loading "lin_tape" tape backend.
c134 LTFS17085I Plugin: Loading "unified" iosched backend.
c134 LTFS14095I Set the tape device write-anywhere mode to avoid cartridge ejection.
c134 LTFS30416I lin_tape version is 3.0.64.
c134 LTFS30423I Opening a device through ibmtape driver (/dev/IBMtape1n).
c134 LTFS30428I Product ID is 'ULTRIUM-HH8     '.
c134 LTFS30429I Vendor ID is IBM     .
c134 LTFS30432I Firmware revision is P381.
c134 LTFS30433I Drive serial is 1097003951.
c134 LTFS17160I Maximum device block size is 1048576.
c134 LTFS11330I Loading cartridge.
c134 LTFS30472I Logical block protection is disabled.
c134 LTFS30457I Cannot get remaining capacity: get log page 0x17 failed (450).
c134 LTFS11332I Load successful.
c134 LTFS30457I Cannot get remaining capacity: get log page 0x17 failed (450).
c134 LTFS17157I Changing the drive setting to write-anywhere mode.
c134 LTFS11005I Mounting the volume.
c134 LTFS30472I Logical block protection is disabled.
c134 LTFS30457I Cannot get remaining capacity: get log page 0x17 failed (450).
c134 LTFS30457I Cannot get remaining capacity: get log page 0x17 failed (450).
c134 LTFS17168E Cannot read volume: medium is not partitioned.
c134 LTFS14013E Cannot mount the volume.
c134 LTFS30472I Logical block protection is disabled.

You can see the result of the bug - the lin_tape_ibmtape_remaining_capacity() function thinks that the lin_tape_ibmtape_logsense() failed because it returned the log page size (450) instead of the error code.

Type of change

Please delete items that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

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

Copy link
Member

@piste-jp piste-jp left a comment

Choose a reason for hiding this comment

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

It looks this fix is not correct.

We changed the prototype of logsense method and it shall return the response length of log page.

The point you should take care is line 2266, the condition shall be if (rc < 0). (See same portion in sg_tape.c)
https://github.com/LinearTapeFileSystem/ltfs/blob/178a782ffed151ff890b41cfdf56f7b8343d3c45/src/tape_drivers/linux/lin_tape/lin_tape_ibmtape.c#L2265-2270

You can see the explanation of the prototype of logsense prototype.

https://github.com/LinearTapeFileSystem/ltfs/blob/178a782ffed151ff890b41cfdf56f7b8343d3c45/src/libltfs/tape_ops.h#L592-604

… to evaluate the return code according to the new semantic, which is negative for error, or positive page size for success
@richard42
Copy link
Contributor Author

okay, thanks for reviewing this. I have pushed changes to the pull request which should fix all of the functions in lin_tape_ibmtape.c which call lin_tape_ibmtape_logsense() with the new return code semantic.

Copy link
Member

@piste-jp piste-jp left a comment

Choose a reason for hiding this comment

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

Hi, Thank you for your additional commit!

It is almost there. I just have one comment. Please let me know if you have opinion against your comment.

src/tape_drivers/linux/lin_tape/lin_tape_ibmtape.c Outdated Show resolved Hide resolved
@piste-jp piste-jp added the to master Merge to master branch label Jun 23, 2023
…c_stioc_command() returns with non-zero code
@richard42
Copy link
Contributor Author

Okay I just pushed another commit to return "rc" instead of -1.

Copy link
Member

@piste-jp piste-jp left a comment

Choose a reason for hiding this comment

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

Thank you very much for your great contribution!

@piste-jp
Copy link
Member

@juliocelon

Please push the Squash and merge button and merge this to the master branch.

@juliocelon juliocelon merged commit 2897a5a into LinearTapeFileSystem:master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants