Hi, I would like to report the serious problem about data loss in customer environment. An OSD data loss has been propagated to other OSDs. If backfill is performed when shard is missing in a primary OSD, the shard that is corresponding to the shard in a primary OSD is also missing in the OSD to which the backfill is directed. In case of 4+2 erasure coding, if copies are occurred against two OSDs during one backfill, three shards are missing(primary + two copies), making data recovery impossible. This data loss depends on setting of erasure coding and the number of copies during backfill. In fact, I could reproduce this situation. This is the actual data loss, and we need to fix this problem. I will verify this with the latest version of ceph, and issue a ticket to redmine later and also report detail information. In this mail, I share simple information of environment and procedure to reproduce at first. Environment: - Ceph version: Nautilus - Erasure coding: 4+2 - Type: filestore Step to Reproduce: 1. Setup more than 6 OSDs (with leaving some extra OSD out). 2. Store some object to pool. 3. Delete a file from a primary OSD in the PG. (In fact, the shard on the primary OSD was unrecognized due to medium error of the primary OSD in the customer environment. To simulate this situation, run `rm`.) e.g.) rm -f /var/lib/ceph/osd/ceph-7/current/1.0s0_head/<some file>.04.21.09\:55\:* 4. Cause backfill in the PG. This time, I could occur backfill by setting OSD to `in` from `out`. e.g.) ceph osd in osd.5 5. ceph -s show active+clean status but object is lost on both primary and backfilled OSDs.
Hi! On Fri, Apr 23, 2021 at 3:53 AM <hase.jin@fujitsu.com> wrote:
Hi, I would like to report the serious problem about data loss in customer environment.
An OSD data loss has been propagated to other OSDs. If backfill is performed when shard is missing in a primary OSD, the shard that is corresponding to the shard in a primary OSD is also missing in the OSD to which the backfill is directed. In case of 4+2 erasure coding, if copies are occurred against two OSDs during one backfill, three shards are missing(primary + two copies), making data recovery impossible. This data loss depends on setting of erasure coding and the number of copies during backfill.
In fact, I could reproduce this situation. This is the actual data loss, and we need to fix this problem. I will verify this with the latest version of ceph, and issue a ticket to redmine later and also report detail information. In this mail, I share simple information of environment and procedure to reproduce at first.
Environment: - Ceph version: Nautilus - Erasure coding: 4+2 - Type: filestore
Step to Reproduce: 1. Setup more than 6 OSDs (with leaving some extra OSD out). 2. Store some object to pool. 3. Delete a file from a primary OSD in the PG. (In fact, the shard on the primary OSD was unrecognized due to medium error of the primary OSD in the customer environment. To simulate this situation, run `rm`.) e.g.) rm -f /var/lib/ceph/osd/ceph-7/current/1.0s0_head/<some file>.04.21.09\:55\:*
This could be causing two different simulated "failures": 1. A listing of objects during recovery doesn't find the object, and thus doesn't recover it. The first case is really a problem with filestore. If the medium error you got led to an incomplete readdir() result from XFS, then Ceph doesn't try to cope with that. 2. The object is in the pg log and recovery tries to read it, but gets ENOENT, and backfill (silently?) skips it. This would be a real problem that affects bluestore as well. An error in this case should either recover from remaining shards (if possible) or log an 'unfound' object. Do you know which of these was triggered by your medium error? sage
Thanks for your reply.
Do you know which of these was triggered by your medium error? This is caused by 1. And I share additional information.
This is caused during backfilling process(not recovering process). I confirmed that missing data was recovered by recovering. So, the problem is data loss when backfilling occurs.
Hi Sage,
If the medium error you got led to an incomplete readdir() result from XFS, then Ceph doesn't try to cope with that.
Do you mean this behavior is in Ceph specifications? It is a problem that data loss actually occurs, so I think we need to solve that. I would like to consider either of the following in Ceph community: -A: Changing to Ceph specifications that does not cause data loss -B: Establish usage or configuration to avoid data loss What do you think about this?
On Mon, Apr 26, 2021 at 4:04 AM <hase.jin@fujitsu.com> wrote:
Hi Sage,
If the medium error you got led to an incomplete readdir() result from XFS, then Ceph doesn't try to cope with that.
Do you mean this behavior is in Ceph specifications? It is a problem that data loss actually occurs, so I think we need to solve that.
I would frame it like this: - With FileStore, Ceph assumed that XFS would return results we could trust (i.e., it would not silently skip files). Trusting XFS turned out to be a bad idea, and not just because of readdir--we also couldn't trust that any data returned by XFS was correct since XFS does not do any sort of data checksums. - We replaced FileStore with BlueStore, which checksums both metadata and data, solving this entire class of problems. The "fix" in this case is to replace your FileStore OSDs with BlueStore. This particular backfill corner case is just one of many bad things that can happen with FileStore and media errors. sage
On Mon, Apr 26, 2021 at 8:09 AM Sage Weil <sage@newdream.net> wrote:
On Mon, Apr 26, 2021 at 4:04 AM <hase.jin@fujitsu.com> wrote:
Hi Sage,
If the medium error you got led to an incomplete readdir() result from XFS, then Ceph doesn't try to cope with that.
Do you mean this behavior is in Ceph specifications? It is a problem that data loss actually occurs, so I think we need to solve that.
I would frame it like this:
- With FileStore, Ceph assumed that XFS would return results we could trust (i.e., it would not silently skip files). Trusting XFS turned out to be a bad idea, and not just because of readdir--we also couldn't trust that any data returned by XFS was correct since XFS does not do any sort of data checksums. - We replaced FileStore with BlueStore, which checksums both metadata and data, solving this entire class of problems.
The "fix" in this case is to replace your FileStore OSDs with BlueStore. This particular backfill corner case is just one of many bad things that can happen with FileStore and media errors.
I agree, bluestore handles such errors in a much better way than filestore and we have further improvements in the pipeline like https://trello.com/c/pWbCyYsz/614-bluestore-make-asserts-unique-per-return-v..., which will help distinguish issues with the underlying layer more easily. Neha
sage _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
Hi Sage, Neha, I agree that we should use bluestore. (All customers will eventually migrate from filestore to bluestore.) However, there is a problem that the migration takes time. Currently my customer runs their system on hundreds of disks, and they cannot shut down the system. So they need to carefully plan their migration and migrate only a few disks in a single migration. Probably it will take more than a year to complete the migration of all the disks. Therefore, I think it is necessary to consider code fixes or operational workarounds for the problems that are detected so that data loss does not occur again by the time the migration is complete. (I understand we should use bluestore, but I think there is still a need to maintain filestore for users who take a long time to migrate.) Does such an idea make sense? For example, I would like to consider the following ideas in detail. 1: In case of operational workarounds We can scrub manually before osd in/out. However, this is an incomplete workaround, as it cannot be address if an automatic backfill occurs in the background. 2: In case of modifying the code. There is no problem with `recovery`. So is it possible to adopt a part of mechanism of `recovery` into `backfill`? (We need to investigate the code in detail.) I'm very happy If you have some idea to solve it. Jin
Hi, Let me share current idea about source code modification. Regarding readdir(), errno is set to indicate the error, so we can check readdir() error in Ceph codes. [readdir() specification] ------------------------------------------------------------ https://man7.org/linux/man-pages/man3/readdir.3.html DESCRIPTION ... It returns NULL on reaching the end of the directory stream or if an error occurred. RETURN VALUE ... If the end of the directory stream is reached, NULL is returned and errno is not changed. If an error occurs, NULL is returned and errno is set to indicate the error. To distinguish end of stream from an error, set errno to zero before calling readdir() and then check the value of errno if NULL is returned. ------------------------------------------------------------ How about modify the codes where readdir () is being executed in filestore as follows: ------------------------------------------------------------ diff --git a/src/os/filestore/BtrfsFileStoreBackend.cc b/src/os/filestore/BtrfsFileStoreBackend.cc index df1d2452a1f..ad0c4d1bb1e 100644 --- a/src/os/filestore/BtrfsFileStoreBackend.cc +++ b/src/os/filestore/BtrfsFileStoreBackend.cc @@ -326,7 +326,13 @@ int BtrfsFileStoreBackend::list_checkpoints(list<string>& ls) list<string> snaps; char path[PATH_MAX]; struct dirent *de; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } snprintf(path, sizeof(path), "%s/%s", get_basedir_path().c_str(), de->d_name); struct stat st; diff --git a/src/os/filestore/FileStore.cc b/src/os/filestore/FileStore.cc index f059331b090..1aeec855e9a 100644 --- a/src/os/filestore/FileStore.cc +++ b/src/os/filestore/FileStore.cc @@ -4987,7 +4987,13 @@ int FileStore::list_collections(vector<coll_t>& ls, bool include_temp) } struct dirent *de = nullptr; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } if (de->d_type == DT_UNKNOWN) { // d_type not supported (non-ext[234], btrfs), must stat struct stat sb; diff --git a/src/os/filestore/LFNIndex.cc b/src/os/filestore/LFNIndex.cc index bbf65bdc66a..530d7501ddc 100644 --- a/src/os/filestore/LFNIndex.cc +++ b/src/os/filestore/LFNIndex.cc @@ -429,7 +429,13 @@ int LFNIndex::list_objects(const vector<string> &to_list, int max_objs, int r = 0; int listed = 0; bool end = true; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } end = false; if (max_objs > 0 && listed >= max_objs) { break; @@ -477,7 +483,13 @@ int LFNIndex::list_subdirs(const vector<string> &to_list, return -errno; struct dirent *de = nullptr; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } string short_name(de->d_name); string demangled_name; if (lfn_is_subdir(short_name, &demangled_name)) { ------------------------------------------------------------ I haven't verified this modification yet, but is this idea (check readdir() errors) acceptable? If Ceph can't detect the error, it's hard to deal with. But regarding readdir(), it can detect the error, so I would like to fix it. Jin
On Wed, Apr 28, 2021 at 5:05 AM <hase.jin@fujitsu.com> wrote:
Hi,
Let me share current idea about source code modification. Regarding readdir(), errno is set to indicate the error, so we can check readdir() error in Ceph codes.
[readdir() specification] ------------------------------------------------------------ https://man7.org/linux/man-pages/man3/readdir.3.html
DESCRIPTION ... It returns NULL on reaching the end of the directory stream or if an error occurred.
RETURN VALUE ... If the end of the directory stream is reached, NULL is returned and errno is not changed. If an error occurs, NULL is returned and errno is set to indicate the error. To distinguish end of stream from an error, set errno to zero before calling readdir() and then check the value of errno if NULL is returned. ------------------------------------------------------------
How about modify the codes where readdir () is being executed in filestore as follows:
------------------------------------------------------------ diff --git a/src/os/filestore/BtrfsFileStoreBackend.cc b/src/os/filestore/BtrfsFileStoreBackend.cc index df1d2452a1f..ad0c4d1bb1e 100644 --- a/src/os/filestore/BtrfsFileStoreBackend.cc +++ b/src/os/filestore/BtrfsFileStoreBackend.cc @@ -326,7 +326,13 @@ int BtrfsFileStoreBackend::list_checkpoints(list<string>& ls) list<string> snaps; char path[PATH_MAX]; struct dirent *de; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + }
Yes! Checking the readdir result is something we should have been doing; ignoring it is a bug. I would adjust this code to also print the error code to the log, though. sage
snprintf(path, sizeof(path), "%s/%s", get_basedir_path().c_str(), de->d_name);
struct stat st; diff --git a/src/os/filestore/FileStore.cc b/src/os/filestore/FileStore.cc index f059331b090..1aeec855e9a 100644 --- a/src/os/filestore/FileStore.cc +++ b/src/os/filestore/FileStore.cc @@ -4987,7 +4987,13 @@ int FileStore::list_collections(vector<coll_t>& ls, bool include_temp) }
struct dirent *de = nullptr; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } if (de->d_type == DT_UNKNOWN) { // d_type not supported (non-ext[234], btrfs), must stat struct stat sb; diff --git a/src/os/filestore/LFNIndex.cc b/src/os/filestore/LFNIndex.cc index bbf65bdc66a..530d7501ddc 100644 --- a/src/os/filestore/LFNIndex.cc +++ b/src/os/filestore/LFNIndex.cc @@ -429,7 +429,13 @@ int LFNIndex::list_objects(const vector<string> &to_list, int max_objs, int r = 0; int listed = 0; bool end = true; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } end = false; if (max_objs > 0 && listed >= max_objs) { break; @@ -477,7 +483,13 @@ int LFNIndex::list_subdirs(const vector<string> &to_list, return -errno;
struct dirent *de = nullptr; - while ((de = ::readdir(dir))) { + while (true) { + errno = 0; + de = ::readdir(dir); + if (de == nullptr) { + ceph_assert(errno == 0); + break; + } string short_name(de->d_name); string demangled_name; if (lfn_is_subdir(short_name, &demangled_name)) { ------------------------------------------------------------
I haven't verified this modification yet, but is this idea (check readdir() errors) acceptable? If Ceph can't detect the error, it's hard to deal with. But regarding readdir(), it can detect the error, so I would like to fix it.
Jin _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
Hi Sage, I confirmed that master has this problem, and issued ticket to redmine. https://tracker.ceph.com/issues/50558
Yes! Checking the readdir result is something we should have been doing; ignoring it is a bug. I would adjust this code to also print the error code to the log, though.
Thank you! I also consider how fix it and continue to verify.
Hi, My colleague issued a PR.(https://github.com/ceph/ceph/pull/41080) I would be very happy if you could review it. Jin
participants (3)
-
hase.jin＠fujitsu.com
-
Neha Ojha
-
Sage Weil