From 2ac21807248008e3a05e6df81c136e33ecd5ffcb Mon Sep 17 00:00:00 2001 From: Dmitry Litvintsev Date: Tue, 19 Dec 2023 09:26:02 -0600 Subject: [PATCH] chimera: Revert "chimera: update FsSqlDriver#inodeOf to throw exception if file not found" Motivation: Issue https://github.com/dCache/dcache/issues/7469 reports that xrood copy to existing fie path with overwrite flag ends up removing destination but no fie replaced. Modification: Just by bisecting this patch has been identified as culprit: 2fb9170283908f911bb7ae462e2ceafd77b25c01. This patch reverts commit 2fb9170283908f911bb7ae462e2ceafd77b25c01 Result: copy to w/ overwrite flag work properly Target: trunk Request: 9.2 Acked-by: Lea Morschel, Paul Millar Patch: https://rb.dcache.org/r/14187/ Require-notes: yes --- .../java/org/dcache/chimera/FsSqlDriver.java | 36 +++++----- .../main/java/org/dcache/chimera/JdbcFs.java | 67 +++++++++++++------ 2 files changed, 61 insertions(+), 42 deletions(-) diff --git a/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java b/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java index 85880402f19..b7869e06a54 100644 --- a/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java +++ b/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java @@ -511,24 +511,21 @@ boolean rename(FsInode inode, FsInode srcDir, String source, FsInode destDir, St * * @param parent * @param name - * @throws FileNotFoundChimeraFsException if name does not exist in parent path - * @return the inode for the named file in the parent directory. + * @return null if path is not found */ - FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) - throws FileNotFoundChimeraFsException { + FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) { switch (name) { case ".": return parent.isDirectory() ? parent : null; case "..": if (!parent.isDirectory()) { - throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); + return null; } FsInode dir = parent.getParent(); return (dir == null) ? parent : dir; default: - FsInode child; if (stat == STAT) { - child = _jdbc.query( + return _jdbc.query( "SELECT c.* FROM t_dirs d JOIN t_inodes c ON d.ichild = c.inumber " + "WHERE d.iparent = ? AND d.iname = ?", ps -> { @@ -538,7 +535,7 @@ FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) rs -> rs.next() ? new FsInode(parent.getFs(), rs.getLong("inumber"), FsInodeType.INODE, 0, toStat(rs)) : null); } else { - child = _jdbc.query("SELECT ichild FROM t_dirs WHERE iparent=? AND iname=?", + return _jdbc.query("SELECT ichild FROM t_dirs WHERE iparent=? AND iname=?", ps -> { ps.setLong(1, parent.ino()); ps.setString(2, name); @@ -546,10 +543,6 @@ FsInode inodeOf(FsInode parent, String name, StatCacheOption stat) rs -> rs.next() ? new FsInode(parent.getFs(), rs.getLong("ichild")) : null); } - if (child == null) { - throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); - } - return child; } } @@ -1620,10 +1613,13 @@ FsInode path2inode(FsInode root, String path) throws ChimeraFsException { */ for (int i = pathElemts.size(); i > 0; i--) { String f = pathElemts.get(i - 1); - try { - inode = inodeOf(parentInode, f, STAT); - } catch (FileNotFoundChimeraFsException e) { - return null; + inode = inodeOf(parentInode, f, STAT); + + if (inode == null) { + /* + * element not found stop walking + */ + break; } /* @@ -1650,7 +1646,7 @@ FsInode path2inode(FsInode root, String path) throws ChimeraFsException { * * @param root staring point * @param path - * @return inode or an empty collection if path does not exist. + * @return inode or null if path does not exist. */ List path2inodes(FsInode root, String path) throws ChimeraFsException { File pathFile = new File(path); @@ -1675,9 +1671,9 @@ List path2inodes(FsInode root, String path) throws ChimeraFsException { /* Path elements are in reverse order. */ for (String f : Lists.reverse(pathElements)) { - try { - inode = inodeOf(parentInode, f, STAT); - } catch (FileNotFoundChimeraFsException e) { + inode = inodeOf(parentInode, f, STAT); + + if (inode == null) { return Collections.emptyList(); } diff --git a/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java b/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java index f13bb19abe7..2d450bdc40a 100644 --- a/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java +++ b/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java @@ -423,6 +423,9 @@ public FsInode createFile(FsInode parent, String name, int owner, int group, int int level = Integer.parseInt(cmd[1]); return inTransaction(status -> { FsInode useInode = _sqlDriver.inodeOf(parent, cmd[2], STAT); + if (useInode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[2]); + } try { Stat stat = useInode.statCache(); return _sqlDriver.createLevel(useInode, stat.getUid(), @@ -559,7 +562,7 @@ public void remove(String path) throws ChimeraFsException { FsInode parent = path2inode(parentPath); String name = filePath.getName(); FsInode inode = _sqlDriver.inodeOf(parent, name, STAT); - if (!_sqlDriver.remove(parent, name, inode)) { + if (inode == null || !_sqlDriver.remove(parent, name, inode)) { throw FileNotFoundChimeraFsException.ofPath(path); } return null; @@ -796,6 +799,9 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); } FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]); + } return new FsInode_ID(this, inode.ino()); } @@ -808,6 +814,9 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) int level = Integer.parseInt(cmd[1]); FsInode inode = _sqlDriver.inodeOf(parent, cmd[2], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[2]); + } if (level <= LEVELS_NUMBER) { stat(inode, level); return new FsInode(this, inode.ino(), level); @@ -908,6 +917,9 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); } FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]); + } return new FsInode_SURI(this, inode.ino()); } @@ -948,6 +960,9 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) } FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]); + } switch (cmd[2]) { case "locality": return new FsInode_PLOC(this, inode.ino()); @@ -970,7 +985,11 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) if (cmd.length != 2) { throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); } - return _sqlDriver.inodeOf(getWormID(), cmd[1], NO_STAT); + FsInode inode = _sqlDriver.inodeOf(getWormID(), cmd[1], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]); + } + return inode; } if (name.startsWith(".(fset)(")) { @@ -980,6 +999,10 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) } FsInode inode = _sqlDriver.inodeOf(parent, cmd[1], NO_STAT); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, cmd[1]); + } + String[] args = new String[cmd.length - 2]; System.arraycopy(cmd, 2, args, 0, args.length); @@ -988,6 +1011,9 @@ public FsInode inodeOf(FsInode parent, String name, StatCacheOption cacheOption) } FsInode inode = _sqlDriver.inodeOf(parent, name, cacheOption); + if (inode == null) { + throw FileNotFoundChimeraFsException.ofFileInDirectory(parent, name); + } fillIdCaches(inode); inode.setParent(parent); return inode; @@ -1130,32 +1156,29 @@ public boolean rename(FsInode inode, FsInode srcDir, String source, FsInode dest throw new NotDirChimeraException(destDir); } - FsInode destInode; - try { - destInode = _sqlDriver.inodeOf(destDir, dest, STAT); - } catch (FileNotFoundChimeraFsException e) { - if (!_sqlDriver.rename(inode, srcDir, source, destDir, dest)) { - throw FileNotFoundChimeraFsException.ofPath(source); + FsInode destInode = _sqlDriver.inodeOf(destDir, dest, STAT); + + if (destInode != null) { + if (destInode.equals(inode)) { + // according to POSIX, we are done + return false; } - return true; - } - if (destInode.equals(inode)) { - // according to POSIX, we are done - return false; - } + /* Renaming into existing is only allowed for the same type of entry. + */ + if (inode.isDirectory() != destInode.isDirectory()) { + throw new FileExistsChimeraFsException(dest); + } - /* Renaming into existing is only allowed for the same type of entry. - */ - if (inode.isDirectory() != destInode.isDirectory()) { - throw new FileExistsChimeraFsException(dest); + if (!_sqlDriver.remove(destDir, dest, destInode)) { + // Concurrent modification - retry + return rename(inode, srcDir, source, destDir, dest); + } } - if (!_sqlDriver.remove(destDir, dest, destInode)) { - // Concurrent modification - retry - return rename(inode, srcDir, source, destDir, dest); + if (!_sqlDriver.rename(inode, srcDir, source, destDir, dest)) { + throw FileNotFoundChimeraFsException.ofPath(source); } - return true; }); }