Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-3840

llapi_layout API design discussion

Details

    • Bug
    • Resolution: Duplicate
    • Major
    • None
    • None
    • None
    • 3
    • 9942

    Description

      Extensions to liblustreapi have been posted for review here.

      http://review.whamcloud.com/5302

      The goal of the API extensions is to provided a user-friendly interface for interacting with the layout of files in Lustre filesystems, and to hide the wire-protocol details behind an opaque data type.

      This issue will provide a forum for potentials users and developers to discuss and critique the design of the proposed API extensions.

      Attachments

        Issue Links

          Activity

            [LU-3840] llapi_layout API design discussion

            I don't see any benefit to abandoning 5302

            We can keep working there. I just find it becomes hard to navigate when the comment and revision history gets too long.

            the xattr interface ... might "succeed" on any filesystem with xattr support, but not actually create the striped file as expected.

            I have not found that to be the case in practice (ext4, zfs, tmpfs, and nfs). I believe (but haven't verified in the code) that a filesystem must explicitly register support for an xattr namespace beyond the standard ones, otherwise the kernel will return EOPNOTSUP.

            nedbass Ned Bass (Inactive) added a comment - I don't see any benefit to abandoning 5302 We can keep working there. I just find it becomes hard to navigate when the comment and revision history gets too long. the xattr interface ... might "succeed" on any filesystem with xattr support, but not actually create the striped file as expected. I have not found that to be the case in practice (ext4, zfs, tmpfs, and nfs). I believe (but haven't verified in the code) that a filesystem must explicitly register support for an xattr namespace beyond the standard ones, otherwise the kernel will return EOPNOTSUP.

            I don't see any benefit to abandoning 5302and creating a new change over just pushing a new patch which drops the changes you don't want. Making a new change just means more places to look for information about this change.

            As for checks, I agree with John. Simple checks against hard (constant) limits are fine, but I'd prefer to do any complex checks (e.g. opening /proc files and iterating) in a separate function.

            The kernel has to do all of these checks itself anyway. The main drawback is that the xattr interface that is needed for BG/L is clumsy because it might "succeed" on any filesystem with xattr support, but not actually create the striped file as expected. The ioctl() interface was less troublesome in this regard since it is unlikely that any filesystem would handle the Lustre ioctl command. It is worthwhile to verify if setxattr("lustre.lov") will work on other filesystems or if they will refuse the "lustre.lov" xattr because it is not in one if the normal namespaces ("user", "system", "trusted", or "security") that are handled by the kernel.

            adilger Andreas Dilger added a comment - I don't see any benefit to abandoning 5302and creating a new change over just pushing a new patch which drops the changes you don't want. Making a new change just means more places to look for information about this change. As for checks, I agree with John. Simple checks against hard (constant) limits are fine, but I'd prefer to do any complex checks (e.g. opening /proc files and iterating) in a separate function. The kernel has to do all of these checks itself anyway. The main drawback is that the xattr interface that is needed for BG/L is clumsy because it might "succeed" on any filesystem with xattr support, but not actually create the striped file as expected. The ioctl() interface was less troublesome in this regard since it is unlikely that any filesystem would handle the Lustre ioctl command. It is worthwhile to verify if setxattr("lustre.lov") will work on other filesystems or if they will refuse the "lustre.lov" xattr because it is not in one if the normal namespaces ("user", "system", "trusted", or "security") that are handled by the kernel.

            That is one reason I want to start over from patch set 23. It was pretty thoroughly reviewed at that stage so I'd like to see it land with only minor changes and do follow-up work in separate patches. I don't see much benefit to breaking up what's already been reviewed though. The main obstacle to landing it is the expensive validity checks that Andreas objects to. But those checks can simply be removed for now, and replaced with improved interfaces as discussed above in subsequent patches. How would that sit with you, Andreas?

            nedbass Ned Bass (Inactive) added a comment - That is one reason I want to start over from patch set 23. It was pretty thoroughly reviewed at that stage so I'd like to see it land with only minor changes and do follow-up work in separate patches. I don't see much benefit to breaking up what's already been reviewed though. The main obstacle to landing it is the expensive validity checks that Andreas objects to. But those checks can simply be removed for now, and replaced with improved interfaces as discussed above in subsequent patches. How would that sit with you, Andreas?
            simmonsja James A Simmons added a comment - - edited

            Okay. I will take all the changes I did in 5302 and place it in the LU-4665 patch. Andreas we will need to submit a different patch on top of Ned's new patch with your idea of a llapi_layout_verify. That is assuming people will accept your idea. Ned new base patch will be fine since it will not be the back end of anything. Further testing of the LU-4665, which will use the layout api with lfs getstripe and setstripe, on my part will expose any problems with Ned's design. Plus the new base patch will not handle the case of DNE directories so that work will have to be developed as well. Much work to be done.

            I promise to break the work I do in LU-4665 into small incremental patch so people can properly review them.

            Perhaps Ned you could consider breaking your patch up into smaller pieces. It is a lot of work.

            simmonsja James A Simmons added a comment - - edited Okay. I will take all the changes I did in 5302 and place it in the LU-4665 patch. Andreas we will need to submit a different patch on top of Ned's new patch with your idea of a llapi_layout_verify. That is assuming people will accept your idea. Ned new base patch will be fine since it will not be the back end of anything. Further testing of the LU-4665 , which will use the layout api with lfs getstripe and setstripe, on my part will expose any problems with Ned's design. Plus the new base patch will not handle the case of DNE directories so that work will have to be developed as well. Much work to be done. I promise to break the work I do in LU-4665 into small incremental patch so people can properly review them. Perhaps Ned you could consider breaking your patch up into smaller pieces. It is a lot of work.

            Yes. John's comments cut to the heart of the matter are in line with what Andy has been asking for. The ultimate test of a layout's validity is whether Lustre accepts and creates it. But the API should provide interfaces that allow the user to determine valid layout values.

            At this point I think http://review.whamcloud.com/#/c/5302/ should be abandoned and replaced with a new change ID based on patch set 23. That was the last stable point in the revision series (aside from errno stomping bugs caught by Frank), and there has been too much churn since then for me to review the changes with any confidence. Future revisions should be based strictly on design changes agreed to here, and avoid unnecessary refactoring of code that has already been reviewed, tested, and debugged.

            For now, I respectfully request to be the only person to push changes to the review. Others are of course welcome to contribute dependent patches as separate reviews. I'm grateful that James took the initiative to move this forward, but I think it's important for consistency's sake to just have one chef in the kitchen. Please communicate any new requirements for LU-4665 integration here so that work can move forward.

            To summarize, I propose that we do the following, in order.

            • Abandon change 5302
            • Submit a new gerrit review based on 5302 patch set 23
            • Identify the questions we need the API to answer as suggested by John
            • Post draft man pages for new interfaces for review here, revise as needed
            • Refresh the patch with implementation of new interfaces, along with complete test cases and documentation

            Are others on board with this plan?

            nedbass Ned Bass (Inactive) added a comment - Yes. John's comments cut to the heart of the matter are in line with what Andy has been asking for. The ultimate test of a layout's validity is whether Lustre accepts and creates it. But the API should provide interfaces that allow the user to determine valid layout values. At this point I think http://review.whamcloud.com/#/c/5302/ should be abandoned and replaced with a new change ID based on patch set 23. That was the last stable point in the revision series (aside from errno stomping bugs caught by Frank), and there has been too much churn since then for me to review the changes with any confidence. Future revisions should be based strictly on design changes agreed to here, and avoid unnecessary refactoring of code that has already been reviewed, tested, and debugged. For now, I respectfully request to be the only person to push changes to the review. Others are of course welcome to contribute dependent patches as separate reviews. I'm grateful that James took the initiative to move this forward, but I think it's important for consistency's sake to just have one chef in the kitchen. Please communicate any new requirements for LU-4665 integration here so that work can move forward. To summarize, I propose that we do the following, in order. Abandon change 5302 Submit a new gerrit review based on 5302 patch set 23 Identify the questions we need the API to answer as suggested by John Post draft man pages for new interfaces for review here, revise as needed Refresh the patch with implementation of new interfaces, along with complete test cases and documentation Are others on board with this plan?
            jhammond John Hammond added a comment -

            Let me rephrase. What are the use cases of validity checking in user space? I'm in favor of good interfaces that answer specific questions like: "How many stripes can I have?" or "What are the minimum and maximum stripe sizes?" These are easy to answer.

            On the other hand, offering an API to answer "Is this striping valid?" makes me a bit uncomfortable. It's a bit like being asked "Where babies come from?" by someone else's kid. There are too many details to ensure that striping that passes this function will be always be accepted and created by Lustre.

            Setting implementation aside, how do I use such a function? Do I create various hypothetical striping and pass them to verify? This seems like a parameter search to answer the questions from the first paragraph.

            jhammond John Hammond added a comment - Let me rephrase. What are the use cases of validity checking in user space? I'm in favor of good interfaces that answer specific questions like: "How many stripes can I have?" or "What are the minimum and maximum stripe sizes?" These are easy to answer. On the other hand, offering an API to answer "Is this striping valid?" makes me a bit uncomfortable. It's a bit like being asked "Where babies come from?" by someone else's kid. There are too many details to ensure that striping that passes this function will be always be accepted and created by Lustre. Setting implementation aside, how do I use such a function? Do I create various hypothetical striping and pass them to verify? This seems like a parameter search to answer the questions from the first paragraph.

            I have to voice a concern that lack of a formal design process is threatening to derail this API. There is too much implementation going on with too little understanding of requirements, as John hinted at. There should be one architect/implementer to preserve unity of design and conceptual integrity, with others providing feedback, requirements, and code review. New public interfaces such as llapi_layout_verify() should be fully documented in man page format and reviewed here before an implementation is submitted for review in gerrit.

            So, let's precisely nail down exactly what our validity checking requirements are before charging ahead with implementation. We need concrete answers to at least:

            • Is the return status of fsetxattr() a sufficient indicator of validity?
            • If supplemental validity checking is needed (beyond fsetxattr()), how much control over that checking needs to be exposed through the API?
            • What specific error conditions need to be communicated?
            nedbass Ned Bass (Inactive) added a comment - I have to voice a concern that lack of a formal design process is threatening to derail this API. There is too much implementation going on with too little understanding of requirements, as John hinted at. There should be one architect/implementer to preserve unity of design and conceptual integrity, with others providing feedback, requirements, and code review. New public interfaces such as llapi_layout_verify() should be fully documented in man page format and reviewed here before an implementation is submitted for review in gerrit. So, let's precisely nail down exactly what our validity checking requirements are before charging ahead with implementation. We need concrete answers to at least: Is the return status of fsetxattr() a sufficient indicator of validity? If supplemental validity checking is needed (beyond fsetxattr()), how much control over that checking needs to be exposed through the API? What specific error conditions need to be communicated?

            The llot_magic is still there. Users will always find a way to break things :-/

            simmonsja James A Simmons added a comment - The llot_magic is still there. Users will always find a way to break things :-/

            At some point I added a llot_magic canary field to use as a minimal sanity check. But I somehow lost that change in all the refreshes. I think we should add a magic field, but I'm opposed to further measures such as checksums to detect or prevent corruption. The application is ultimately responsible for not trashing its memory.

            nedbass Ned Bass (Inactive) added a comment - At some point I added a llot_magic canary field to use as a minimal sanity check. But I somehow lost that change in all the refreshes. I think we should add a magic field, but I'm opposed to further measures such as checksums to detect or prevent corruption. The application is ultimately responsible for not trashing its memory.
            jhammond John Hammond added a comment -

            What is the use case of llapi_layout_verfiy()?

            jhammond John Hammond added a comment - What is the use case of llapi_layout_verfiy()?
            simmonsja James A Simmons added a comment - - edited

            Okay my local patch uses a path argument :

            int llapi_layout_verify(const struct llapi_layout *layout, const char *path)

            For llapi_file_open what I'm doing currently is

            {
                   /* If the user verifed the layout we still need to ensure that the
                     * requested file is located on the same lustre file system as the
                     * layout. */
                    if (strlen(layout->llot_fsname) != 0) {
                            /* Verify that the file path belongs to a lustre filesystem. */
                            rc = llapi_search_fsname(path, fsname);
                            if (rc < 0 || (strlen(fsname) == 0)) {
                                    errno = ENOTTY;
                                    return -1;
                            }
            
                            /* Verify the file system of the user supplied path is the 
                             * same one the layout was created for. */
                            if (strcmp(fsname, layout->llot_fsname)) {
                                    errno = ENOTTY;
                                    return -1;
                            }
                    } else if (llapi_layout_verify(layout, path) < 0)
                            return -1;
            

            So it forces a verify if it doesn't see llot_fsname. If verified we still need to make sure that the file is located on the same file system. A subdirectory could be another file system mount. So I handle my fsname paranoia.

            As for corruption, well that is harder to deal with. For checksum we have to write a check sum algothrim since userland libcfs is going away. Also with that small amount of data their is greater chance of checksum value collisons. Maybe we could get away with a hashtable. Again with libcfs going away that might be more challanging. Have to see if we can use just the header. Currently struct llapi_layout is opaque but I guess nothing stops the user from casting and scribbling.

            simmonsja James A Simmons added a comment - - edited Okay my local patch uses a path argument : int llapi_layout_verify(const struct llapi_layout *layout, const char *path) For llapi_file_open what I'm doing currently is { /* If the user verifed the layout we still need to ensure that the * requested file is located on the same lustre file system as the * layout. */ if (strlen(layout->llot_fsname) != 0) { /* Verify that the file path belongs to a lustre filesystem. */ rc = llapi_search_fsname(path, fsname); if (rc < 0 || (strlen(fsname) == 0)) { errno = ENOTTY; return -1; } /* Verify the file system of the user supplied path is the * same one the layout was created for . */ if (strcmp(fsname, layout->llot_fsname)) { errno = ENOTTY; return -1; } } else if (llapi_layout_verify(layout, path) < 0) return -1; So it forces a verify if it doesn't see llot_fsname. If verified we still need to make sure that the file is located on the same file system. A subdirectory could be another file system mount. So I handle my fsname paranoia. As for corruption, well that is harder to deal with. For checksum we have to write a check sum algothrim since userland libcfs is going away. Also with that small amount of data their is greater chance of checksum value collisons. Maybe we could get away with a hashtable. Again with libcfs going away that might be more challanging. Have to see if we can use just the header. Currently struct llapi_layout is opaque but I guess nothing stops the user from casting and scribbling.

            People

              nedbass Ned Bass (Inactive)
              nedbass Ned Bass (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: