Merge pull request #360 from WaberZhuang/main

Fix CI & upper layer bug

Author: BigVan

.github/workflows/project-checks.yml

@@ -25,7 +25,50 @@ jobs:
           path: src/github.com/containerd/overlaybd
           fetch-depth: 100
 
-      - name: Project checks
-        uses: containerd/project-checks@v1.1.0
-        with:
-          working-directory: src/github.com/containerd/overlaybd
+      - name: set env
+        shell: bash
+        run: |
+          echo "GOPATH=${{ github.workspace }}" >> $GITHUB_ENV
+          echo "${{ github.workspace }}/bin" >> $GITHUB_PATH
+
+      - name: Install dependencies
+        shell: bash
+        env:
+          GO111MODULE: on
+        run: |
+          echo "::group::🚧 Get dependencies"
+          go install -v github.com/vbatts/git-validation@latest
+          go install -v github.com/containerd/ltag@latest
+          echo "::endgroup::"
+
+      - name: DCO Checks
+        shell: bash
+        working-directory: src/github.com/containerd/overlaybd
+        env:
+          GITHUB_COMMIT_URL: ${{ github.event.pull_request.commits_url }}
+          REPO_ACCESS_TOKEN: ""
+          DCO_VERBOSITY: "-v"
+          DCO_RANGE: ""
+        run: |
+          echo "::group::👮 DCO checks"
+          set -x
+          if [[ ! -z "${REPO_ACCESS_TOKEN}" ]]; then
+          HEADERS=(-H "Accept: application/vnd.github+json" -H "Authorization: Bearer ${REPO_ACCESS_TOKEN}")
+          else
+          HEADERS=(-H "Accept: application/vnd.github+json")
+          fi
+          if [ -z "${GITHUB_COMMIT_URL}" ]; then
+          DCO_RANGE=$(jq -r '.after + "..HEAD"' ${GITHUB_EVENT_PATH})
+          else
+          DCO_RANGE=$(curl "${HEADERS[@]}" ${GITHUB_COMMIT_URL} | jq -r '.[0].parents[0].sha + "..HEAD"')
+          fi
+          git-validation ${DCO_VERBOSITY} ${DCO_RANGE} -run DCO,short-subject,dangling-whitespace
+          echo "::endgroup::"
+
+      - name: Validate file headers
+        shell: bash
+        working-directory: src/github.com/containerd/overlaybd
+        run: |
+          echo "::group::📚 File headers"
+          ltag -t "script/validate/template" --excludes "vendor contrib" --check -v
+          echo "::endgroup::"

.github/workflows/release.yml

@@ -31,10 +31,10 @@ jobs:
       uses: actions/checkout@v3
       with:
         submodules: true
+    - name: Set up QEMU
+      uses: docker/setup-qemu-action@v3
     - name: Setup buildx instance
-      uses: docker/setup-buildx-action@v2
-      with:
-        use: true
+      uses: docker/setup-buildx-action@v3
     - name: Build
       shell: bash
       run: |
@@ -84,10 +84,17 @@ jobs:
         ls -l releases/
       env:
         IMAGE: ${{ matrix.images }}
+    - id: upload_name
+      name: Upload name
+      run: |
+        image=${{ matrix.images }}
+        platform=${{ matrix.platforms }}
+        name="releases-${image//\//_}-${platform/\//_}"
+        echo "name=${name}" >> "$GITHUB_OUTPUT"
     - name: Upload
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4
       with:
-        name: releases
+        name: ${{ steps.upload_name.outputs.name }}
         path: releases/overlaybd-*.*
 
   dev-release:
@@ -97,7 +104,7 @@ jobs:
     needs: [build]
     steps:
       - name: Download builds and release notes
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
       - name: Display downloaded files
         shell: bash
         run: ls -l releases
@@ -118,7 +125,7 @@ jobs:
     needs: [build]
     steps:
       - name: Download builds and release notes
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
       - name: Display downloaded files
         shell: bash
         run: ls -l releases

src/image_file.cpp

@@ -481,6 +481,7 @@ int ImageFile::init_image_file() {
         goto ERROR_EXIT;
     }
 
+    // have only RO layers
     if (upper.index() == "" || upper.data() == "") {
         LOG_INFO("RW layer path not set. return RO layers.");
         m_file = lower_file;
@@ -493,29 +494,36 @@ int ImageFile::init_image_file() {
         LOG_ERROR("open upper layer failed.");
         goto ERROR_EXIT;
     }
-    // We have to maintain the lower_file because prefetcher need the readonly
-    // lower_file but not the writable m_file.
-    //
-    // Otherwise, lower_file and upper_file will lose the ownership of m_files
-    // after stack_files(...), so we could safely delete them.
+
+    // have only one RW layer
+    if (!lower_file) {
+        LOG_INFO("RO layers path not set. return RW layer.");
+        m_file = upper_file;
+        read_only = false;
+        goto SUCCESS_EXIT;
+    }
+
+    // stack_files(..., ownership=true) will destruct lower_file and upper_file
+    // immediately, but the read-only lower_file is needed by prefetcher, so we
+    // have to use stack_files(..., ownership=false) instead.
     //
-    // upper_file will be deleted immediately since it's useless.
-    // lower_file will be held by ImageFile until deconstruct.
+    // For this reason, lower_file and upper_file must be maintained until m_file
+    // is deconstructed.
     stack_ret = LSMT::stack_files(upper_file, lower_file, false, false);
     if (!stack_ret) {
         LOG_ERROR("LSMT::stack_files(`, `)", (uint64_t)upper_file, true);
         goto ERROR_EXIT;
     }
     m_file = stack_ret;
     read_only = false;
-    delete upper_file;
     m_lower_file = lower_file;
+    m_upper_file = upper_file;
 
 SUCCESS_EXIT:
     if (conf.download().enable() && !record_no_download) {
         start_bk_dl_thread();
     }
-    if (m_prefetcher != nullptr) {
+    if (m_prefetcher && lower_file) {
         m_prefetcher->replay(lower_file);
     }
     return 1;

src/image_file.h

@@ -62,9 +62,8 @@ class ImageFile : public photon::fs::ForwardFile {
             m_file->close();
             delete m_file;
         }
-        if (m_lower_file) {
-            delete m_lower_file;
-        }
+        if (m_lower_file) delete m_lower_file;
+        if (m_upper_file) delete m_upper_file;
     }
 
     int fstat(struct stat *buf) override {
@@ -121,6 +120,7 @@ class ImageFile : public photon::fs::ForwardFile {
     photon::join_handle *dl_thread_jh = nullptr;
     ImageService &image_service;
     photon::fs::IFile *m_lower_file = nullptr;
+    photon::fs::IFile *m_upper_file = nullptr;
 
     int init_image_file();
     template<typename...Ts> void set_failed(const Ts&...xs);

src/overlaybd/lsmt/file.cpp

@@ -1807,7 +1807,7 @@ IFileRW *stack_files(IFileRW *upper_layer, IFileRO *lower_layers, bool ownership
         rst = new LSMTWarpFile;
         delta++;
     }
-    idx = create_combo_index((IMemoryIndex0 *)u->m_index, l->m_index, l->m_files.size(), true);
+    idx = create_combo_index((IMemoryIndex0 *)u->m_index, l->m_index, l->m_files.size(), ownership);
     rst->m_index = idx;
     rst->m_findex = u->m_findex;
     rst->m_vsize = u->m_vsize;
@@ -1830,9 +1830,9 @@ IFileRW *stack_files(IFileRW *upper_layer, IFileRO *lower_layers, bool ownership
     }
     rst->m_uuid.push_back(u->m_uuid[0]);
     rst->m_rw_tag = rst->m_files.size() - delta;
-    u->m_index = l->m_index = nullptr;
-    l->m_file_ownership = u->m_file_ownership = false;
     if (ownership) {
+        u->m_index = l->m_index = nullptr;
+        l->m_file_ownership = u->m_file_ownership = false;
         delete u;
         delete l;
     }

src/overlaybd/tar/erofs/test/erofs_stress_base.cpp

@@ -352,7 +352,7 @@ LSMT::IFileRW *StressBase::mkfs()
 
 		LSMT::IFileRW *img_file = nullptr;
 		if (i > 0)
-			img_file = LSMT::stack_files(current_layer, lowers, false, false);
+			img_file = LSMT::stack_files(current_layer, lowers, true, false);
 		else
 			img_file = current_layer;
 
@@ -362,7 +362,6 @@ LSMT::IFileRW *StressBase::mkfs()
 			delete img_file;
 			LOG_ERROR_RETURN(-1, nullptr, "fail to extract tar");
 		}
-		delete lowers;
 		delete tar;
 		lowers = img_file;
 	}