Re: [PATCH 1/7] open_by_handle: store and load file handles from file

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Jan 11, 2018 at 1:59 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Sun, Jan 07, 2018 at 08:07:19PM +0200, Amir Goldstein wrote:
>> usage:
>>  open_by_handle -p -o <handles_file> <test_dir> [N]
>>  open_by_handle -p -i <handles_file> <test_dir> [N]
>>
>> This will be used to test decoding of file handles after various
>> file systems operations including mount cycle.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> I found that it might be easier to review if the new test that takes use
> of this new function is included in the same patch, so reviewer could
> know how are these new functions being used without switching between
> different patches.
>

OK. I will give it a shot to see how that works out.

>> ---
>>  src/open_by_handle.c | 110 +++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 85 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
>> index dbc5b0f..f9dfefc 100644
>> --- a/src/open_by_handle.c
>> +++ b/src/open_by_handle.c
>> @@ -43,30 +43,36 @@ Examples:
>
> The usage info above the "Examples:" can be updated too.
>
> "usage: open_by_handle [-cludmrwapk] <test_dir> [num_files]"
>

OK.

>>
>>     open_by_handle -p <test_dir> [N]
>>
>> -3. Get file handles for existing test set, write data to files,
>> +3. Get file handles for existing test set and write them to a file.
>> +   Read file handles from file and open files by handle:
>> +
>> +   open_by_handle -p -o <handles_file> <test_dir> [N]
>> +   open_by_handle -p -i <handles_file> <test_dir> [N]
>> +
>> +4. Get file handles for existing test set, write data to files,
>>     drop caches, open all files by handle, read and verify written
>>     data, write new data to file:
>>
>>     open_by_handle -rwa <test_dir> [N]
>>
>> -4. Get file handles for existing test set, unlink all test files,
>> +5. Get file handles for existing test set, unlink all test files,
>>     remove test_dir, drop caches, try to open all files by handle
>>     and expect ESTALE:
>>
>>     open_by_handle -dp <test_dir> [N]
>>
>> -5. Get file handles for existing test set, keep open file handles for all
>> +6. Get file handles for existing test set, keep open file handles for all
>>     test files, unlink all test files, drop caches and try to open all files
>>     by handle (should work):
>>
>>     open_by_handle -dk <test_dir> [N]
>>
>> -6. Get file handles for existing test set, rename all test files,
>> +7. Get file handles for existing test set, rename all test files,
>>     drop caches, try to open all files by handle (should work):
>>
>>     open_by_handle -m <test_dir> [N]
>>
>> -7. Get file handles for existing test set, hardlink all test files,
>> +8. Get file handles for existing test set, hardlink all test files,
>>     then unlink the original files, drop caches and try to open all
>>     files by handle (should work):
>>
>> @@ -103,7 +109,7 @@ struct handle {
>>
>>  void usage(void)
>>  {
>> -     fprintf(stderr, "usage: open_by_handle [-cludmrwapk] <test_dir> [num_files]\n");
>> +     fprintf(stderr, "usage: open_by_handle [-cludmrwapk] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
>>       fprintf(stderr, "\n");
>>       fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
>>       fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
>> @@ -116,6 +122,8 @@ void usage(void)
>>       fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
>>       fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
>>       fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
>> +     fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
>> +     fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
>>       exit(EXIT_FAILURE);
>>  }
>>
>> @@ -131,15 +139,16 @@ int main(int argc, char **argv)
>>       char    *test_dir;
>>       char    *mount_dir;
>>       int     mount_fd, mount_id;
>> +     int     in_fd = 0, out_fd = 0;
>>       int     numfiles = 1;
>>       int     create = 0, delete = 0, nlink = 1, move = 0;
>>       int     rd = 0, wr = 0, wrafter = 0, parent = 0;
>>       int     keepopen = 0;
>>
>> -     if (argc < 2 || argc > 4)
>> +     if (argc < 2)
>>               usage();
>>
>> -     while ((c = getopt(argc, argv, "cludmrwapk")) != -1) {
>> +     while ((c = getopt(argc, argv, "cludmrwapki:o:")) != -1) {
>>               switch (c) {
>>               case 'c':
>>                       create = 1;
>> @@ -176,13 +185,27 @@ int main(int argc, char **argv)
>>               case 'k':
>>                       keepopen = 1;
>>                       break;
>> +             case 'i':
>> +                     in_fd = open(optarg, O_RDONLY);
>> +                     if (in_fd < 0) {
>> +                             perror(optarg);
>> +                             return EXIT_FAILURE;
>> +                     }
>> +                     break;
>> +             case 'o':
>> +                     out_fd = creat(optarg, 0644);
>> +                     if (out_fd < 0) {
>> +                             perror(optarg);
>> +                             return EXIT_FAILURE;
>> +                     }
>> +                     break;
>>               default:
>>                       fprintf(stderr, "illegal option '%s'\n", argv[optind]);
>>               case 'h':
>>                       usage();
>>               }
>>       }
>> -        if (optind == argc || optind > 2)
>> +        if (optind == argc)
>>              usage();
>>       test_dir = argv[optind++];
>>       if (optind < argc)
>> @@ -192,11 +215,14 @@ int main(int argc, char **argv)
>>               usage();
>>       }
>>
>> -     if (parent) {
>> +     if (parent && !in_fd) {
>>               strcpy(dname, test_dir);
>>               /*
>>                * -p flag implies that test_dir is NOT a mount point,
>>                * so its parent can be used as mount_fd for open_by_handle_at.
>> +              * -i flag implies that test_dir IS a mount point, because we
>> +              *  are testing open by handle of dir, which may have been
>> +              *  deleted or renamed.
>
> I'm a bit confused by this comment, is test_dir a mount point if I
> specify both -i and -p? Does that mean -i would override -p regarding to
> test_dir being mount point or not?
>

Yes, I elaborated the documentation.
For a generic utility, the mount point argument should have been split
to a different optional option and default to same value as test_dir, but
I rather keep the test arguments simple, so here goes:

        /*
         * The way we determine the mount_dir to be used for mount_fd argument
         * for open_by_handle_at() depends on other command line arguments:
         *
         * -p flag usually (see -i below) implies that test_dir is NOT a mount
         *    point, but a directory inside a mount point that we will create
         *    and/or encode/decode during the test, so we use test_dir's parent
         *    for mount_fd. Even when not creatig test_dir, if we would use
         *    test_dir as mount_fd, then drop_caches will not drop the test_dir
         *    dcache entry.
         *
         * If -p is not specified, we don't have a hint whether test_dir is a
         *    mount point or not, so we assume the worst case, that it is a
         *    mount point and therefore, we cannnot use parent as mount_fd,
         *    because parent may be on a differnt file system.
         *
         * -i flag, even with -p flag, implies that test_dir IS a mount point,
         *    because we are testing open by handle of dir, which may have been
         *    deleted or renamed and we are not creating nor encoding the
         *    directory file handle. -i flag is meant to be used for tests
         *    after encoding file handles and mount cycle the file system. If
         *    we would require the test to pass in with -ip the test_dir we
         *    want to decode and not the mount point, that would have populated
         *    the dentry cache and the use of -ip flag combination would not
         *    allow testing decode of dir file handle in cold dcache scenario.
         */


>>                */
>>               mount_dir = dirname(dname);
>>               if (create)
>> @@ -241,15 +267,24 @@ int main(int argc, char **argv)
>>       /* sync to get the new inodes to hit the disk */
>>       sync();
>>
>> -     /* create the handles */
>> +     /* create/read the handles */
>
> This loop also write handles out, update the comment accordingly?
>

OK.

>>       for (i=0; i < numfiles; i++) {
>>               sprintf(fname, "%s/file%06d", test_dir, i);
>> -             handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>> -             ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
>> -             if (ret < 0) {
>> -                     strcat(fname, ": name_to_handle");
>> -                     perror(fname);
>> -                     return EXIT_FAILURE;
>> +             if (in_fd) {
>> +                     ret = read(in_fd, (char *)&handle[i], sizeof(*handle));
>> +                     if (ret < sizeof(*handle)) {
>> +                             strcat(fname, ": read handle");
>> +                             perror(fname);
>> +                             return EXIT_FAILURE;
>> +                     }
>> +             } else {
>> +                     handle[i].fh.handle_bytes = MAX_HANDLE_SZ;
>> +                     ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
>> +                     if (ret < 0) {
>> +                             strcat(fname, ": name_to_handle");
>> +                             perror(fname);
>> +                             return EXIT_FAILURE;
>> +                     }
>>               }
>>               if (keepopen) {
>>                       /* Open without close to keep unlinked files around */
>> @@ -260,15 +295,40 @@ int main(int argc, char **argv)
>>                               return EXIT_FAILURE;
>>                       }
>>               }
>> +             if (out_fd) {
>> +                     ret = write(out_fd, (char *)&handle[i], sizeof(*handle));
>> +                     if (ret < sizeof(*handle)) {
>> +                             strcat(fname, ": write handle");
>> +                             perror(fname);
>
> A short write is not an error, so errno is 0 and error message can be
> confusing: "$file: write handle: Success"

OK. fixed all of those.

Thanks,
Amir.